-
Notifications
You must be signed in to change notification settings - Fork 111
Authentication overhaul #129
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
Authentication overhaul #129
Conversation
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.
Backward compatibility is not maintained.
This will break every single piecs of code written agains the library, likely causing more work that we can handle. Not good.
It's been a number of months since I wrote the code for this particular PR but IIRC |
@AR-Calder Yes, I have added the import statement into the HubRestApi.py and is seems to be back to functional state. Functional parity of blackduck.Client with HubRestApi is less critical at the moment than keeping the restructured HubRestApi interface and functionally identical to the existing one. There are new features that old library, and the code that uses it, can benefit from. And we should try to introduce it as transparently as possible. Modifying internal structure of the library is pretty much a fair game, as long as the compatibility is maintained. On the other hand, modifying external facing features of the library should to be based on specific goals that are explicitly stated and verified. It is preferable for modifications to be additive for compatibility reasons. For example, the re-authentication is at the top of the list to allow for long running programs based on this library. Our current interface does not allow to talk to multiple instances of a Blackduck without constantly re-initializing HubInstance object. It would be nice to have a concise justification for the new external interface with benefits and downsides elaborated to sufficient depth. That will allow to maintain focus. We know the downsides - we have ask our customers to rewrite their code, the code that does not otherwise have problems. We have to state the upsides - we are changing the library interface dramatically - because it somehow will make users life better in some way Thoughts? |
They don't necessarily have to change their existing code - you could leave the existing interface in for backwards compatibility. Just think for a different approach it would be best to use a new interface, rather than trying to conform to the restrictions of the current one.
These sorts of changes represent a different approach - in my opinion, methods of a given interface should be idiomatic. In this case the cleanest way to achieve this is to create an alternative interface - which also ensures existing projects with this dependency do not break. Happy to discuss further with you @mkumykov and @gsnyder2007 on a call. |
Agreed, new interface has a promise to be better. And by all means, let's make it better. |
Perhaps it might be best to leave |
I am a bit wary of that approach. We can't drop old code, and we cant create new code fast enough. |
Authentication overhaul providing similar functionality to 118 but using the proper requests
auth
hook.Additionally provides request wrapper to provide automatic pagination of applicable resources, header insertion, etc.
Demo of how these can be used in
test/demo_client.py
.