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.

Sign up for GitHub

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

Jump to bottom

Support mirror registries with path component #1707

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Support mirror registries with path component #1707

merged 1 commit into from
Aug 11, 2021

Conversation

imjasonh
Copy link
Collaborator

Fixes #1618

Description

This change should allow --mirror-registry=my.mirror/some/prefix/path, where previously only repository roots were supported.

This PR doesn't currently include tests that cover this behavior, if there's a good place to add that please let me know.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

--mirror-registry now supports path components

@google-cla google-cla bot added the cla: yes CLA signed by all commit authors label Jul 28, 2021
@imjasonh imjasonh mentioned this pull request Jul 28, 2021
Closed
@imjasonh
Copy link
Collaborator Author

Tests seem to fail with this error message:

        error building image: GET https://gcr.io/v2/kaniko-test/extraction-base-image/manifests/latest: UNAUTHORIZED: You don't have the needed permissions to perform this operation, and you may have invalid credentials. To authenticate your request, follow the steps in: https://cloud.google.com/container-registry/docs/advanced-authentication

This seems unrelated to my change, unless I'm missing something. Is it a flake?

tejal29
tejal29 requested changes Aug 11, 2021
View reviewed changes
Copy link
Member

@tejal29 tejal29 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution.
Please add tests!

@tejal29
Copy link
Member

tejal29 commented Aug 11, 2021

Tests seem to fail with this error message:

        error building image: GET https://gcr.io/v2/kaniko-test/extraction-base-image/manifests/latest: UNAUTHORIZED: You don't have the needed permissions to perform this operation, and you may have invalid credentials. To authenticate your request, follow the steps in: https://cloud.google.com/container-registry/docs/advanced-authentication

This seems unrelated to my change, unless I'm missing something. Is it a flake?

Sorry @imjasonh, somehow internal test project got deleted :(. Doing my best to get it moved and recover.

tejal29
tejal29 approved these changes Aug 11, 2021
View reviewed changes
Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tejal29
Copy link
Member

tejal29 commented Aug 11, 2021

Thank you for your contribution.
Please add tests!

nvm. This is not needed

@tejal29 tejal29 merged commit 4e218f2 into GoogleContainerTools:master Aug 11, 2021
This was referenced Oct 20, 2021
Open
Open
tejal29 added a commit to tejal29/kaniko that referenced this pull request Oct 20, 2021
@tejal29
Revert "Support mirror registries with path component (GoogleContaine…
9fdc7a1 
…rTools#1707)"

This reverts commit 4e218f2.
tejal29 added a commit that referenced this pull request Oct 21, 2021
@tejal29
Revert "Support mirror registries with path component (#1707)" (#1794)
ee31dc9 
* Revert "Support mirror registries with path component (#1707)"

This reverts commit 4e218f2.

* fix tests and add co-sign back
@davidc davidc mentioned this pull request Mar 27, 2022
Closed
@allen0099 allen0099 mentioned this pull request Apr 14, 2023
Closed
@lionello
Copy link
Contributor

@imjasonh Do you think this can be fixed to avoid the regressions from the first PR? See #1790 #1791 #1793

@Realiserad
Copy link

@tejal29 Why was this reverted? Any plans to add this back again or should we open a new PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tejal29 tejal29 tejal29 approved these changes

Assignees
No one assigned
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot use --registry-mirror with a mirror that requires a prefix
4 participants
@imjasonh @tejal29 @lionello @Realiserad