Skip to content

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

Merged

Conversation

OffBy0x01
Copy link
Collaborator

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.

@gsnyder2007 gsnyder2007 requested a review from mkumykov March 9, 2021 23:11
Copy link
Contributor

@mkumykov mkumykov left a 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.

@OffBy0x01
Copy link
Collaborator Author

It's been a number of months since I wrote the code for this particular PR but IIRC HubRestApi.py should still work as normal - but if not please let me know. The Client class I reference is intended to be an alternative that can gradually achieve functional parity with the existing HubRestApi client - would require a refactor to large for a single PR.

@mkumykov
Copy link
Contributor

@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?

@OffBy0x01
Copy link
Collaborator Author

OffBy0x01 commented Mar 11, 2021

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.

  • Personally I'd like the new interface to be more dynamic, by which I mean taking full advantage of HATEOAS - it should be possible to simply provide generic functions e.g. bd.get_resource(name='projects') as opposed to bd.get_projects() , and bd.get_resource(project, 'versions') as opposed to bd.get_project_versions(project).
  • Support for user-controlled http client settings inc proxy.
  • No/less hard-coded headers.
  • As a consequence of the above - API less likely to break with hub updates.
  • Endpoints supporting pagination can return generators directly providing items i.e. do not have to get object['items']; enabling user to do things like for version in bd.get_resource(project, 'versions'): print(version.get('versionName')

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.

@mkumykov
Copy link
Contributor

Agreed, new interface has a promise to be better. And by all means, let's make it better.
Maintaining old interfaces is a shield that will allow sane cadence for new development.

@OffBy0x01
Copy link
Collaborator Author

Perhaps it might be best to leave HubRestApi.py as it was and leave the class splitting to the new interface - thoughts?

@mkumykov
Copy link
Contributor

I am a bit wary of that approach. We can't drop old code, and we cant create new code fast enough.
It might create resource contention with one of the competing code bases going stale.
I like the approach that you have taken. You can maintain old interface until we decide to drop it.
And you can plug existing methods into the new code and make them useable, while working on better written replacements. An evolutionary approach, if you will.
Exactly what you have done so far. We just put some due diligence tests on top of it and we have a winner.

@gsnyder2007 gsnyder2007 merged commit 42ddff6 into blackducksoftware:master Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants