-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix bearer plugin OCI cloud registries issues #7308
Fix bearer plugin OCI cloud registries issues #7308
Conversation
289c2e2
to
5414806
Compare
Thanks @carabasdaniel! It would be great if we had a test to exercise this if possible. |
53fb052
to
01f3b2f
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Can you please squash your commits. Are you also able to test this with ACR? |
b381a5a
to
b6426f5
Compare
Squashed and rebased on latest main. I'm having some trouble with access to the azure portal at the moment and that is why I requested for someone to test the PR. I'll try to test later today if I can get access to ACR |
Hi @ashutosh-narkar, I finally managed to test this fix with ACR and it seems to work as expected with a standard private container registry. OPA is able to download the policy image and load the bundle as expected. |
… a redirect Signed-off-by: carabasdaniel <[email protected]>
b6426f5
to
d915874
Compare
@@ -153,6 +157,9 @@ func (ap *bearerAuthPlugin) NewClient(c Config) (*http.Client, error) { | |||
|
|||
func (ap *bearerAuthPlugin) Prepare(req *http.Request) error { | |||
token := ap.Token | |||
if ap.logger == nil { | |||
ap.logger = logging.Get() |
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 think the logger should be provided. This Get
call is deprecated. Just noting this if we have to remove it later.
This change should fix #7092, #7189 and #6590
I've tested this with AWS, but I would appreciate if someone can please test this with ACR