Skip to content
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

Add preset headers to get_manifest request #121

Closed
wants to merge 8 commits into from

Conversation

kavish-p
Copy link
Contributor

Currently when making the request to get manifests, previously set headers are ignored and only the Allowed media type headers are passed to do_request.

I have a scenario where I set the Bearer token in the client using self._client.set_token_auth(token)

However, the call for get_manifest fails as the Authorization header is not passed. It falls back to using Basic authentication in do_request, which fails because my registry only support token authentication.

@vsoch
Copy link
Contributor

vsoch commented Jan 26, 2024

Thanks for opening this @kavish-p - note that we need to debug what looks like a Python 3.12 issue: https://stackoverflow.com/questions/77364550/attributeerror-module-pkgutil-has-no-attribute-impimporter-did-you-mean.

For the time being, my concern is that the headers are not updated for a reason. For example, data might get stale there, and so it was an implicit decision (for some registry) to refresh them. This means that although it might fix your particular use case, it could break someone else's.

What I'd like to ask you to do is have a boolean to give to the client on it, something like refresh_headers with default set to True that indicates the current (default) state to not run the update from the previous. Then for your use case you can set that variable to False, meaning they are not refreshed (and you carry forward the old information). That way the client will stay as it is but solve your issue (and others like it).

And if you find a fix to this new bug, would be appreciated too! I thought it was related to black but (unfortunately looks to be a Python 3.12 deprecation).

@vsoch
Copy link
Contributor

vsoch commented Jan 26, 2024

@kavish-p please rebase with master to get a fix for the formatting issue. Then next step would be to address/discuss the comments for the PR. Thank you!

vsoch added 3 commits January 29, 2024 11:19
* update black to 24.1.0 and pin python to 3.11

There is a bug with using conda (3.12) and a removed dependency
that is needed for setuptools. The solution for now is to use the install
python action and pin to 3.11.

Signed-off-by: vsoch <[email protected]>
@vsoch
Copy link
Contributor

vsoch commented Jan 29, 2024

@kavish-p a few things to work on while this is testing:

  • You'll need to sign the CLA to contribute to ORAS, since it's under the Linux fonudation. You can click the DCO action check -> details for how to do that.
  • You'll need to bump the version in oras/version.py
  • Add a corresponding note to the change log

You'll also want to make sure that when refresh_headers is not defined (None) it gets set to False so we aren't having either the case of True or None, but rather True or False.

Signed-off-by: Kavish Punchoo <[email protected]>
Signed-off-by: Kavish Punchoo <[email protected]>
Signed-off-by: Kavish Punchoo <[email protected]>
@vsoch
Copy link
Contributor

vsoch commented Jan 29, 2024

Also @kavish-p you need to rebase against master - my commits shouldn't be in there.

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.

2 participants