Bug 1708648: pkg/oc/cli/admin/release/new: Guard against inconsistent sources #22816
Conversation
openshift-ci-robot
added
the
size/S
|
/hold until we identify what the ART fix is (which might be using this to gate sync) |
openshift-ci-robot
added
the
do-not-merge/hold
Under what ART outcome would we not want this guard? Even if we are confident that we will never do this as a result of some ART/CI fix, the guard won't hurt us, and it might help other folks who are cutting their own releases with this tooling. |
|
So a case this blocks which I'm not positive I want to blanket give up on is where we decide to rebuild a particular image for a specific fix without touching anything else. The image stream is our composition tool. We do some checks in oc for sanity, but this is a very strong check that may have legitimate cases. If we put this in here, we can't bypass it later. |
Add a |
wking
force-pushed
the
release-ensure-consistent-commits
branch
from
3094f69
to
d1aebc9
Compare
openshift-ci-robot
added
size/M
|
/retest Now that Friday's Route 53 meltdown has been resolved. |
wking
force-pushed
the
release-ensure-consistent-commits
branch
from
d1aebc9
to
c9d8e9d
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking If they are not already assigned, you can assign the PR to them by writing 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 |
|
Bumped the completions to fix the verify failure with d1aebc9 -> c9d8e9d. |
|
unit: /retest |
|
unit: Since resolved by changes I didn't follow closely enough to understand ;). /retest |
|
unit: /retest |
|
All green now :). |
wking
force-pushed
the
release-ensure-consistent-commits
branch
from
c9d8e9d
to
0a1557b
Compare
wking
changed the title
|
/retest |
|
unit: /retest |
|
unit: /retest |
|
unit: /retest |
|
All green :). |
| @@ -1466,6 +1475,32 @@ func pruneEmptyDirectories(dir string) error { | |||
| }) | |||
| } | |||
|
|
|||
| // ensureConsistentSources checks that images built from the same Git repository are from the same Git commit. | |||
| func ensureConsistentSources(is *imageapi.ImageStream) error { | |||
There was a problem hiding this comment.
nit: this function should return the error with all the different commits for a location, not the difference to first found....
There was a problem hiding this comment.
nit: this function should return the error with all the different commits for a location...
Dup of your other comment? I'll mark this resolved based on the change I referenced there, but please re-open if there is another facet of this that I'm overlooking.
There was a problem hiding this comment.
For example today MCO has 4 images.
When all 4 differ error will be something like
mcc was xx mco was x for mco repo
mcs was xy mco was x for mco repo
mcd was xz mco was x for mco repo.
But I guess a better error message would be
mco repo has inconsistent commits mco x mcc xx mcs xy mcd xz
There was a problem hiding this comment.
When all 4 differ error will be something like...
Yeah, I'm fine with that. Doesn't seem like something that is going to come up often, and we can always come back and polish the error message if it does.
Sometimes folks attempt to build release images with images built from different commits from the same Git repository. This can sometimes break the release [1] and is almost always a bad idea. This commit fails fast in this situation, because it's harder to root-cause this if we fail later on. Folks who want to take responsibility for any divergence can set --allow-commit-divergence, in which case we warn and continue instead of erroring out. And of course, the real fix for folks who want to avoid this is to fix the source you're pointing oc at, this guard in oc is just covering your back ;). Completions bumped with: $ make build WHAT=cmd/oc $ hack/update-generated-completions.sh I'm personally fine with ensureConsistentSources returning only the first difference found, because I don't expect there to be many within a given source stream. But Abhinav asked for all of them [2], so I'm using NewAggregate to collect them. From the NewAggregate docs [3]: If the slice is empty, this returns nil. so we don't need a len(errs) guard locally. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1708648 [2]: openshift#22816 (comment) [3]: https://godoc.org/k8s.io/apimachinery/pkg/util/errors#NewAggregate
wking
force-pushed
the
release-ensure-consistent-commits
branch
from
0a1557b
to
2d09425
Compare
|
unit: /test unit |
|
/retest |
|
All green, just waiting on approval and a hold-pull :). |
|
@wking: PR needs rebase. 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. |
openshift-ci-robot
added
the
needs-rebase
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci-robot
added
lifecycle/rotten
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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 understand the commands that are listed here. |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. 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. |
Sometimes folks attempt to build release images with images built from different commits from the same Git repository. This can sometimes break the release and is almost always a bad idea. This commit fails fast in this situation, because it's harder to root-cause this if we fail later on. I'd be ok with a flag to allow divergence for callers who feel like they know what they're doing,
but have not added it in this commit[edit: now I have]. And of course, the real fix is to not do this ;), although I don't think that means this oc code shouldn't guard against it.