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
Bug 1766287: Improve usability of hello-openshift imagestream #350
Conversation
@yselkowitz: This pull request references Bugzilla bug 1766287, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 |
2 similar comments
/retest |
/retest |
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.
/hold
@yselkowitz over Slack you asked, "Why don't we set lookupPolicy: local: true
on everything?", and at the time I didn't have a good reason not to.
Now that the PR is in front of me, I have found a reason why we haven't set this field. Our convention for imagestream tag pull specs can overlap with actual Docker Hub images. In the case of oath-proxy
and hello-openshift
we would replace the docker hub image with the one from the payload on upgrade. To me, this is a breaking change.
Unfortunately this means that @cyril-ui-developer is out of luck here. His next best option is to add the alpha.image.policy.openshift.io/resolve-names: '*'
annotation to the Deployment's pod template spec.
lookupPolicy: | ||
local: true |
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.
openshift/oauth-proxy resolves to a Docker Hub image. There is a risk that an end-user is pulling the (obsolete) oauth-proxy image from Docker Hub today, and with this switch will start pulling the image from the internal registry.
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.
That probably would be a good thing, but let's leave that for further discussion in a separate PR.
lookupPolicy: | ||
local: true |
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.
Likewise, openshift/hello-openshift resolves to a Docker Hub image.
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.
That is actually the entire point of the (new in 4.7) hello-openshift imagestream, to easily replace uses of the unmaintained, single-arch, and now rate-limited image on docker.io.
Repushed the PR limiting the changes to hello-openshift for now. |
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.
/hold cancel
/approve
@yselkowitz please squash your commits. |
This is necessary to allow a Deployment to use it with little to no change: https://austindewey.com/2020/10/25/you-can-use-openshifts-imagestreams-with-deployments-heres-how/ Also, update hello-openshift pullspec in CVO manifests to point to the OKD builds on quay.io like the others.
Squashed as requested. |
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
Tested via cluster-bot and the dev console. Image replacement still works when the alpha.image.policy.openshift.io/resolve-names: '*'
annotation is removed.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, yselkowitz 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
@yselkowitz: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with Bugzilla bug 1766287 has not been moved to the MODIFIED state. 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. |
I asked for this image to be removed from the product in openshift/enhancements#438 - I really don't want to be adding stuff around this. Ingress should be using their operator image and a subcommand of their operator binary, not depending on a generic example image. |
OK @smarterclayton I've subscribed to that EP (I see @yselkowitz already responded to you there on current usage of this imagestream) and ack that I'm now officially aware of your wishes My first thought is that I need to invesitigate / understand when the existing multi arch / console / serverless uses will be replaced so as not to break them. Or do you want this imagestream removed now as a way to trigger their replacement? |
/assign @adambkaplan