[1.20] pull: do check for blocked registries #5034
Conversation
Do not check whether a registry is blocked. c/image is taking care of that implicitly when attempting to copy an image. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
openshift-ci
bot
added
release-note
openshift-ci
bot
added
the
approved
|
/cc @mtrmac @vrothberg |
|
@haircommander: GitHub didn't allow me to request PR reviews from the following users: mtrmac. Note that only cri-o members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
|
@haircommander: once the present PR merges, I will cherry-pick it on top of release-1.19 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
.Blocked is also evaluated below in the unqualified-image case. I feel quite strongly that the two should behave the same.
IIRC this behavior difference was historically known when implementing the c/image approach, and the CRI-O code here was left around intentionally at that time, though I’m not 100% certain and I might be misremembering. Is there a specific rationale for either that old decision, or the new decision?
(OTOH I don’t feel strongly about preserving this behavior at all, I’m happy to defer decisions about CRI-O behavior stability to CRI-O maintainers. “The code will be simpler and consistent with other c/image users” might be a perfectly good justification.)
...for the unqualified registry use-case, as c/image can calculate it Signed-off-by: Peter Hunt <pehunt@redhat.com>
My updating this code was mostly because the current behavior wrt the interaction of blocked and mirrored registries is not intuitive to me. I am not married to defering the decision to c/image, but "simpler and consistent" are both also good reasons to do so :) I removed the .Blocked handling below as well |
The concept of blocking was created before c/image was aware of it, and IIRC before mirrors were a thing. So it’s quite possible that this before-mirror blocking was just inherited after mirrors, and left around because nobody wanted to affirmatively decide make a change to the behavior, i.e. this might have never been a conscious decision. |
|
/retest |
|
@saschagrunert ptal |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, saschagrunert, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@haircommander: new pull request created: #5040 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Do not check whether a registry is blocked. c/image is taking care of
that implicitly when attempting to copy an image.
Signed-off-by: Valentin Rothberg rothberg@redhat.com
What type of PR is this?
/kind bug
What this PR does / why we need it:
or else mirrored registries cannot resolve there
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?