Skip to content

OTA-817: Support ClusterVersion spec DesiredUpdate Architecture field #860

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 2 commits into from
Dec 20, 2022
Merged

OTA-817: Support ClusterVersion spec DesiredUpdate Architecture field #860

openshift-merge-robot merged 2 commits into from
Dec 20, 2022

Conversation

@jottofar
Copy link
Contributor

@jottofar jottofar commented Nov 3, 2022

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2022
@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, 2022
@jottofar jottofar force-pushed the ota-817 branch 3 times, most recently from 89c7b11 to e58cfd1 Compare November 4, 2022 15:34
@jottofar
Copy link
Contributor Author

jottofar commented Nov 4, 2022

/test e2e-agnostic

@jottofar jottofar force-pushed the ota-817 branch 2 times, most recently from c7c008b to 42e1bf0 Compare November 8, 2022 14:36
@jottofar
vendor: Update openshift/api to pickup Architecture
00997af 
Steps:

$ go get github.com/openshift/api@a671aa8
$ go mod tidy
$ go mod vendor
@jottofar
Copy link
Contributor Author

jottofar commented Dec 8, 2022 •
edited by openshift-ci bot
Loading

/retitle OTA-817: Support ClusterVersion spec DesiredUpdate Architecture field

@openshift-ci openshift-ci bot changed the title WIP: OTA-817: Support ClusterVersion spec DesiredUpdate Architecture field OTA-817: Support ClusterVersion spec DesiredUpdate Architecture field Dec 8, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2022
@jottofar jottofar force-pushed the ota-817 branch 2 times, most recently from 182059e to 528421a Compare December 8, 2022 21:51
@jottofar
Copy link
Contributor Author

jottofar commented Dec 9, 2022

/test e2e-agnostic-upgrade-out-of-change

wking
wking reviewed Dec 12, 2022
View reviewed changes
pkg/cvo/availableupdates.go
if update != nil {
return string(update.Architecture)
}
return ""
Copy link
Member

@wking wking Dec 12, 2022

Choose a reason for hiding this comment

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

you could probably put the "default to current arch" fallback in here, although I don't think it matters much if you'd rather keep that defaulting up in the caller level.

@jottofar jottofar force-pushed the ota-817 branch 5 times, most recently from 62399bc to 5730ebf Compare December 15, 2022 17:22
@jottofar
Copy link
Contributor Author

jottofar commented Dec 15, 2022

/retest

1 similar comment
@jottofar
Copy link
Contributor Author

jottofar commented Dec 19, 2022

/retest

petr-muller
petr-muller reviewed Dec 19, 2022
View reviewed changes
Copy link
Member

@petr-muller petr-muller left a comment

Choose a reason for hiding this comment

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

LGTM with a nit, leaving tag for Trevor who was here first

@jottofar
Copy link
Contributor Author

jottofar commented Dec 19, 2022

Trying this again before overriding since e2e-agnostic-upgrade-out-of-change finally worked.

flake: job name "agnostic-upgrade-into-change" does not have network type in name (expected ovn)

/test e2e-agnostic-upgrade-into-change

wking
wking approved these changes Dec 19, 2022
View reviewed changes
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Both Petr and my quibbles are things we can consider addressing in follow-up work, and shouldn't block this improvement from landing.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jottofar, 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-ci-robot
Copy link
Contributor

openshift-ci-robot commented Dec 19, 2022

/retest-required

Remaining retests: 0 against base HEAD 4f9883a and 2 for PR HEAD 5730ebf in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 20, 2022

@jottofar: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit af08109 into openshift:master Dec 20, 2022
@petr-muller petr-muller mentioned this pull request Dec 20, 2022
Merged
@petr-muller petr-muller mentioned this pull request Apr 20, 2023
Merged
@jianlinliu
Copy link

jianlinliu commented May 4, 2023

/label cherry-pick-approved

@openshift-ci openshift-ci bot added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wking wking wking approved these changes

@LalatenduMohanty LalatenduMohanty Awaiting requested review from LalatenduMohanty

+1 more reviewer

@petr-muller petr-muller petr-muller left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@wking wking

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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.

6 participants

@jottofar @openshift-ci-robot @jianlinliu @wking @petr-muller @openshift-merge-robot