Skip to content

Bug 1878925: pkg/cvo/updatepayload: Drop history from target pullspec lookup#684

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
wking:drop-history-lookup
Nov 4, 2021
Merged

Bug 1878925: pkg/cvo/updatepayload: Drop history from target pullspec lookup#684
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
wking:drop-history-lookup

Conversation

@wking
Copy link
Copy Markdown
Member

@wking wking commented Nov 3, 2021

The logic I'm removing is a convenience from 558ae02 (#63). But the current consensus is that:

  • Rollbacks are risky enough that we don't want to make them that convenient.
  • Rollbacks have never been supported, so removing the convenient pullspec lookup is a backwards-compatible-enough change.

This also brings the ClusterVersion operator's approach into line with how oc adm upgrade --to ... has always worked; openshift/oc#566.

@wking
pkg/cvo/updatepayload: Drop history from target pullspec lookup
c80bd02 
The logic I'm removing is a convenience from 558ae02 (cvo: Record
history of changes to version and report target version, 2019-01-08, openshift#63).
But the current consensus is that:

* Rollbacks are risky enough that we don't want to make them that
  convenient.
* Rollbacks have never been supported, so removing the convenient
  pullspec lookup is a backwards-compatible-enough change.

This also brings the ClusterVersion operator's approach into line with
how 'oc adm upgrade --to ...' has always worked [1].

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1878925
@openshift-ci openshift-ci Bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 3, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 3, 2021

@wking: This pull request references Bugzilla bug 1878925, which is invalid:

  • expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1878925: pkg/cvo/updatepayload: Drop history from target pullspec lookup

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 openshift-ci Bot requested review from jottofar and vrutkovs November 3, 2021 17:52
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2021
@wking
Copy link
Copy Markdown
Member Author

wking commented Nov 3, 2021

/bugzilla refresh

@openshift-ci openshift-ci Bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Nov 3, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 3, 2021

@wking: This pull request references Bugzilla bug 1878925, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.10.0) matches configured target release for branch (4.10.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

Details

In response to this:

/bugzilla refresh

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 openshift-ci Bot requested a review from zhouying7780 November 3, 2021 17:54
vrutkovs
vrutkovs approved these changes Nov 4, 2021
View reviewed changes
Copy link
Copy Markdown

@vrutkovs vrutkovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrutkovs, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 4, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 8cc2aad into openshift:master Nov 4, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 4, 2021

@wking: 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 refresh.

Bugzilla bug 1878925 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 1878925: pkg/cvo/updatepayload: Drop history from target pullspec lookup

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.

@wking wking deleted the drop-history-lookup branch November 5, 2021 03:42
@openshift-ci openshift-ci Bot mentioned this pull request Nov 5, 2021
Merged
wking added a commit to wking/cluster-version-operator that referenced this pull request Jan 19, 2023
@wking
lib/validation/validation: Drop "a previous version" from desiredUpda…
e866b29 
…te validation

Narrowly catching up with c80bd02 (pkg/cvo/updatepayload: Drop
history from target pullspec lookup, 2021-11-03, openshift#684), which removed
that option from the implementation.  Having the validation code
living so far from the implementation code makes us more vulnerable to
this kind of divergence, but I'll leave larger refactorings to
follow-up work.
@wking wking mentioned this pull request Jan 19, 2023
Merged
wking added a commit to wking/cluster-version-operator that referenced this pull request Jan 19, 2023
@wking
lib/validation: Drop "a previous version" from desiredUpdate validation
7690401 
Narrowly catching up with c80bd02 (pkg/cvo/updatepayload: Drop
history from target pullspec lookup, 2021-11-03, openshift#684), which removed
that option from the implementation.  Having the validation code
living so far from the implementation code makes us more vulnerable to
this kind of divergence, but I'll leave larger refactorings to
follow-up work.
DavidHurta pushed a commit to DavidHurta/cluster-version-operator that referenced this pull request Jan 31, 2023
@wking @DavidHurta
lib/validation: Drop "a previous version" from desiredUpdate validation
bb6acd8 
Narrowly catching up with c80bd02 (pkg/cvo/updatepayload: Drop
history from target pullspec lookup, 2021-11-03, openshift#684), which removed
that option from the implementation.  Having the validation code
living so far from the implementation code makes us more vulnerable to
this kind of divergence, but I'll leave larger refactorings to
follow-up work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jottofar jottofar Awaiting requested review from jottofar

@zhouying7780 zhouying7780 Awaiting requested review from zhouying7780

1 more reviewer

@vrutkovs vrutkovs vrutkovs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vrutkovs vrutkovs

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@wking @openshift-bot @vrutkovs @openshift-merge-robot