-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Move dockerRegistry tag patch into with dockerRegistry section to avoid build break with image tags #1083
Conversation
Signed-off-by: Andrew Leonard <[email protected]>
Thank you for creating a pull request!Please check out the information below if you have not made a pull request here before (or if you need a reminder how things work). Code Quality and Contributing GuidelinesIf you have not done so already, please familiarise yourself with our Contributing Guidelines and Code Of Conduct, even if you have contributed before. TestsGithub actions will run a set of jobs against your PR that will lint and unit test your changes. Keep an eye out for the results from these on the latest commit you submitted. For more information, please see our testing documentation. In order to run the advanced pipeline tests (executing a set of mock pipelines), it requires an admin to post |
run tests |
@mahdipub fyi |
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'm struggling a little to understand the fix here so just to clarify - is this fix going to work on the assumption that things from a separate registry (i.e. with DOCKER_REGISTRY
in the pipeline config) won't have a tag or is it more subtle than that?
Correct, the problem @mahdipub was trying to fix was when using a dockerRegistry rather than the default dockerhub, so for his purpose the fix only needs to be when DOCKER_REGISTRY is used |
The proper fix I suspect is to re-work the pipeline code to correctly use the docker context with a specific Registry |
If they have a tag, then you are right that fix will not work either..... but I am guessing @mahdipub semeru does not use "tags" ? |
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.
Correct, the problem @mahdipub was trying to fix was when using a dockerRegistry rather than the default dockerhub, so for his purpose the fix only needs to be when DOCKER_REGISTRY is used
Gotcha - thanks for confirming
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
PR TESTER RESULT ✅ All pipelines passed! ✅ |
Fixes: #1082
A patch to fix the above build break, fix #1076 was added to handle dockerRegistry and adding a local tage of DOCKER_IMAGE, but does not correctly handle the fact the image may have a :tag or @digest
This patch moves that code to the dockerRegistry section, but the proper fix for #1076 should be to not do that tagging, but instead probably to use context.docker.withRegistry when necessary to run the container, but that needs proper investigation.