-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
explicitly prohibit updates to imagestream tags that are not spec tags #18532
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@smarterclayton @deads2k ptal the net effect of this is that if you edit an IST that was only pushed, the update results in adding a new spec tag to the IS:
Which near as i can tell is absolutely required because that's where the annotations are stored, so you can't add annotations w/o introducing a spec tag. I would certainly rather this go through the mainline defaulting logic instead of what i'm doing, but it sounds like that might not be a realistic option. |
(the other option is that we just say "you shouldn't be updating ISTs that were only pushed, the current behavior is acceptable") |
I don't actually know how other clients will interpret this. Without an import policy, it doesn't sound that bad. |
@smarterclayton bump |
one bad thing about this: since "From" is empty, it results in rather user unfriendly errors when you run "oc import-image", e.g.:
Clearly having a spec.tag with an empty "From" is not handled well by oc import-image. So this may not be the right solution. (In theory i could also craft the "From" value to point to the internal registry, but that seems a bit dubious) /hold |
7904374
to
d9ec94f
Compare
@deads2k @smarterclayton ok i've reworked this to just reject the update w/ a clean message telling you why your imagestreamtag update is being rejected. ptal again. /hold cancel |
d9ec94f
to
be881c9
Compare
be881c9
to
4619b6c
Compare
/retest |
@smarterclayton bump |
implicit approval from @smarterclayton and @deads2k, lgtm'ing /hold |
Makes me sad, but I doubt we fix it now. lgetm |
/cherry-pick release-3.9 |
@deads2k I think it's /cherrypick and anyway I don't deem this critical enough to backport to 3.9 at this point (just in case this new error behavior causes unexpected problems, i'd rather let it soak for a release). |
/hold cancel |
/retest |
Automatic merge from submit-queue (batch tested with PRs 18587, 18296, 18667, 18665, 18532). |
fixes #18519