-
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
Add support for private registry with custom port in restic-helper image #1999
Conversation
…age definition Signed-off-by: Roman Klimenko <cognoz@ya.ru>
Thanks for the PR @cognoz! We do need you to add DCO signoff to your commit -- |
…age definition Signed-off-by: Roman Klimenko <cognoz@ya.ru>
@cognoz I think the code looks reasonable. I would like to add a couple new test cases though:
You should be able to just copy/paste and modify from the existing test cases. Once that's done, I think we can merge this. |
@cognoz thanks for making the updates, this LGTM! One last request - could you add a changelog with a brief description of the bug fix? See https://velero.io/docs/master/code-standards/ for details. |
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.
LGTM pending changelog.
cc @carlisia @prydonius @nrb - PTAL |
Changelog added, thx for guidelines! |
@carlisia @nrb @prydonius ping - this one's waiting for review/merge. |
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 like the new test, but is there a possibility for a case where the config map could haev invalid data in the key? If possible I'd like to make it so the modified test is an additional test, and keep the case for config map with invalid data in 'image' key returns default image
.
@carlisia if you look at the code you'll notice that we're no longer trying to split image names based on |
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.
👍
…age (vmware-tanzu#1999) * Add support for private registry with custom port in restic-helper image definition Signed-off-by: Roman Klimenko <cognoz@ya.ru>
Closes #1972
example of image definition with custom registry port in CM: