-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VIRA-293: allow access token auth #77
Conversation
"max_retries": 2, | ||
"options": option_kwargs, | ||
} | ||
kwargs.update(auth_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of hate the way way I construct the final kwargs array, but I didn't want to import some deep-dict-merge module for this. So right now it's a bit ratty and probably fails cyclomatic complexity scores with the janky if/else loop earlier.
I don't think it's quite yet shit enough to refactor :P I do think that dictionary.update sucks by default and doesn't do what one expects (sensible recursive merge ala jsonpatch).
Sorry I have been "away" for a bit. I am going to take a look at all the new posted issues then a real look into this one. |
I am currently getting the error: No `vira_servers.json/yaml` file found or, check the config! See `README.md` for more information.
Error detected while processing function vira#_set[19]..vira#_connect:
line 14:
Traceback (most recent call last):
File "<string>", line 1, in <module>
File ".../github/jacobstr/vira/python/Vira/vira_api.py", line 198, in connect
auth_kwargs['basic_auth'] == (username, password)
KeyError: 'basic_auth' I am sure there is a quick fix for it but I am going to look into why it turned backwards on us to try and catch it with the proper |
I even knew I didn't look close enough at it. The core of the problem for the existing users will be the Keeping it working would be the nicest I can do the merge and the fix or you can do the fix if you would like. If it still works for both of us I will release it to |
Heh thanks @n0v1c3 - I'm fine if you patch it up since you were able to repro it and will be on deck to validate the fix. I ran through a few scenarios by hand but clearly missed this. |
I assumed it was as simple as that. I wall get it done today and VIRA-293 can just be done with. @jacobstr Please keep finding more to make it even safer for our users. |
@jacobstr I currently have an open branch due to development VIRA-293 should exist until the full merge is complete. |
I implemented this feature a few months ago. |
Fixes VIRA-293: Allow Personal Access Token (PAT) Auth #76