Skip to content

fix(openshift): use env var instead of clusterversion status #2817

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
merged 1 commit into from
Aug 2, 2022
Merged

fix(openshift): use env var instead of clusterversion status #2817

merged 1 commit into from
Aug 2, 2022

Conversation

tylerslaton
Copy link
Contributor

@tylerslaton tylerslaton commented Jul 15, 2022 •
edited
Loading

Description of the change:
Introducing some code that Nick wrote a fix for prior to him leaving Red Hat. Here are his comments:

Get the current OpenShift release version from the OPENSHIFT_RELEASE
environment variable since the behavior of the original source --
the ClusterVersion desired release status field -- has changed.

Note: This is for a high priority bug fix downstream.

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Bug fixes are accompanied by regression test(s)
  • e2e tests and flake fixes are accompanied evidence of flake testing, e.g. executing the test 100(0) times
  • tech debt/todo is accompanied by issue link(s) in comments in the surrounding code
  • Tests are comprehensible, e.g. Ginkgo DSL is being used appropriately
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky and have an issue
  • Code is properly formatted

@openshift-ci openshift-ci bot requested review from njhale and oceanc80 July 15, 2022 16:27
@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch from 8b88857 to 4bc71f3 Compare July 15, 2022 16:30
@tylerslaton tylerslaton requested a review from timflannagan July 15, 2022 16:34
@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch from 4bc71f3 to 8064bd3 Compare July 15, 2022 17:00
grokspawn
grokspawn approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@grokspawn grokspawn left a comment

Choose a reason for hiding this comment

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

Looks good. Nice work.

@timflannagan
Copy link
Member

@tylerslaton Can you investigate those unit test failures:

••
------------------------------
• [FAILED] [20.053 seconds]
ClusterOperator controller
/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/pkg/controller/operators/openshift/clusteroperator_controller_test.go:19
[It] should gate OpenShift upgrades

Those test failures seem relevant to these set of changes.

@tylerslaton
Copy link
Contributor Author

@tylerslaton Can you investigate those unit test failures:

Yep, been doing that this morning and should have a fix out soon.

timflannagan
timflannagan reviewed Jul 20, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Quick passthrough of the changes here. I'm a bit hesitant with using package-level variables here, and noted some comments inline.

IIRC, this controller watches the ClusterVersion resource: is that watch still applicable in this new implementation?

@tylerslaton
Copy link
Contributor Author

tylerslaton commented Jul 25, 2022 •
edited
Loading

IIRC, this controller watches the ClusterVersion resource: is that watch still applicable in this new implementation?

There is no longer a need to watch the ClusterVersion resource, you're right. Let me see if there is any dead code revolving around that.

@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch 10 times, most recently from b436942 to 3a9b0b9 Compare July 25, 2022 19:54
timflannagan
timflannagan reviewed Jul 25, 2022
View reviewed changes
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Forgot to publish my incomplete review before we starting talking in slack.

@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch 2 times, most recently from ec90969 to fb9f7b5 Compare July 25, 2022 20:35
awgreene
awgreene reviewed Jul 25, 2022
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Overall this looks good, nice work @tylerslaton

@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch from 2d47d1e to 6599a95 Compare July 26, 2022 04:37
@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch 4 times, most recently from f35669e to 3c57ae7 Compare July 27, 2022 17:05
@perdasilva
Copy link
Collaborator

Is it worth having an e2e test, somehow? Could we have a regression test that matches the bug and ensure this fixes it?

@tylerslaton
Copy link
Contributor Author

Is it worth having an e2e test, somehow? Could we have a regression test that matches the bug and ensure this fixes it?

I updated the E2E suite in this PR to test using the env var instead of testing the same logic with the CV. Do you think that would be enough here?

@timflannagan
Copy link
Member

I pulled down these changes and didn't see any more "clusterversion" usage, outside of e2e test utilities, so I think we're covered on that front now.

@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch from 3c57ae7 to 8804dba Compare July 29, 2022 18:44
@tylerslaton
Copy link
Contributor Author

@awgreene @perdasilva @timflannagan How is this looking to you all? I was wondering if we could try to get this through today but let me know if anything is blocking.

exdx
exdx approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@exdx exdx 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 Aug 1, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022
@timflannagan
Copy link
Member

/hold
/approve

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2022
@openshift-ci
Copy link

openshift-ci bot commented Aug 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: exdx, grokspawn, timflannagan, tylerslaton

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@njhale
fix(openshift): use env var instead of clusterversion status
ec9903d 
Get the current OpenShift release version from the RELEASE_VERSION
environment variable since the behavior of the original source --
the ClusterVersion desired release status field -- has changed.

Signed-off-by: Tyler Slaton <[email protected]>
Co-authored-by: Nick Hale <[email protected]>
@tylerslaton tylerslaton force-pushed the env-for-clusterversion branch from 8804dba to ec9903d Compare August 1, 2022 20:42
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022
@timflannagan
Copy link
Member

/lgtm

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

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2022
@openshift-bot
Copy link
Contributor

/retest-required

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

@timflannagan timflannagan merged commit b2086bd into operator-framework:master Aug 2, 2022
@tylerslaton tylerslaton deleted the env-for-clusterversion branch August 2, 2022 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perdasilva perdasilva perdasilva left review comments

@awgreene awgreene awgreene left review comments

@timflannagan timflannagan timflannagan approved these changes

@exdx exdx exdx approved these changes

@grokspawn grokspawn grokspawn approved these changes

@njhale njhale Awaiting requested review from njhale

@oceanc80 oceanc80 Awaiting requested review from oceanc80

Assignees

@awgreene awgreene

@timflannagan timflannagan

@exdx exdx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
7 participants
@tylerslaton @timflannagan @perdasilva @openshift-bot @awgreene @exdx @grokspawn