Skip to content

HIVE-2871 - Always show the upgradeable annotation.#2705

Merged
2uasimojo merged 1 commit into
openshift:masterfrom
AlexVulaj:HIVE-2871_always-show-upgradeable-annotation
Jun 17, 2025
Merged

HIVE-2871 - Always show the upgradeable annotation.#2705
2uasimojo merged 1 commit into
openshift:masterfrom
AlexVulaj:HIVE-2871_always-show-upgradeable-annotation

Conversation

@AlexVulaj
Copy link
Copy Markdown
Contributor

@AlexVulaj AlexVulaj commented Jun 16, 2025

Follow up to HIVE-2819 - always show the upgradeable annotation. If the annotation is present with an empty string, this means that the cluster is upgradeable.

@AlexVulaj AlexVulaj changed the title HIVE-2819 - Always show the upgradeable annotation. HIVE-2871 - Always show the upgradeable annotation. Jun 16, 2025
@openshift-ci openshift-ci Bot requested review from 2uasimojo and dlom June 16, 2025 20:06
Comment thread pkg/controller/clusterversion/clusterversion_controller_test.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@8796c4f). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2705   +/-   ##
=========================================
  Coverage          ?   50.29%           
=========================================
  Files             ?      287           
  Lines             ?    33976           
  Branches          ?        0           
=========================================
  Hits              ?    17087           
  Misses            ?    15540           
  Partials          ?     1349           
Files with missing lines Coverage Δ
...roller/clusterversion/clusterversion_controller.go 45.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@2uasimojo 2uasimojo left a comment

Choose a reason for hiding this comment

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

I agree with Trevor at least that the label=>annotation change should be made, but since that's just in unit tests, I'm happy to let this land as is and see that change in a fup.

/lgtm

...but I'll

/hold

so you can control that cadence however you want (re-push and I'll re-stamp; or unhold and fup).

}),
validate: func(t *testing.T, cd *hivev1.ClusterDeployment) {
assert.Equal(t, "", cd.Annotations[constants.MinorVersionUpgradeUnavailable], "unexpected version major-minor-patch label")
assert.Equal(t, "", cd.Annotations[constants.MinorVersionUpgradeUnavailable], "Unexpected value for label hive.openshift.io/minor-version-upgrade-unavailable")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would have to dig into why this test was working before (as opposed to having to assert that there was no such key) but it seems accurate in its new form 🤷

Copy link
Copy Markdown
Contributor Author

@AlexVulaj AlexVulaj Jun 16, 2025

Choose a reason for hiding this comment

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

The cd.Annotations map gets set to a non-nil value in the functional part of the code. In Go, trying to pull a map entry by a key that doesn't exist will return the 0 value of that map's value type. For example:

myMap := map[string]string{}
foo := myMap["foo"]

in this case, foo will be an empty string.

The old test probably should've been asserting instead that the key didn't exist instead of looking at the value.

@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 Jun 16, 2025
@openshift-ci openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 16, 2025
…he annotation is present with an empty string, this means that the cluster is upgradeable.
@AlexVulaj AlexVulaj force-pushed the HIVE-2871_always-show-upgradeable-annotation branch from 9892ef1 to 8e15858 Compare June 16, 2025 22:16
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2025
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 17, 2025

@AlexVulaj: 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-sigs/prow repository. I understand the commands that are listed here.

@2uasimojo
Copy link
Copy Markdown
Member

/lgtm

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

openshift-ci Bot commented Jun 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, AlexVulaj

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

@AlexVulaj
Copy link
Copy Markdown
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 Jun 17, 2025
@AlexVulaj
Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@2uasimojo
Copy link
Copy Markdown
Member

/retest

@2uasimojo
Copy link
Copy Markdown
Member

/retest hive-mce-29-on-pull-request

@2uasimojo
Copy link
Copy Markdown
Member

/retest hive-on-pull-request

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 17, 2025

@2uasimojo: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test coverage
/test e2e
/test e2e-azure
/test e2e-gcp
/test e2e-openstack
/test e2e-pool
/test e2e-vsphere
/test images
/test periodic-images
/test security
/test unit
/test verify

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-hive-master-coverage
pull-ci-openshift-hive-master-e2e
pull-ci-openshift-hive-master-e2e-pool
pull-ci-openshift-hive-master-images
pull-ci-openshift-hive-master-periodic-images
pull-ci-openshift-hive-master-security
pull-ci-openshift-hive-master-unit
pull-ci-openshift-hive-master-verify
Details

In response to this:

/retest hive-mce-29-on-pull-request

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-sigs/prow repository.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 17, 2025

@2uasimojo: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test coverage
/test e2e
/test e2e-azure
/test e2e-gcp
/test e2e-openstack
/test e2e-pool
/test e2e-vsphere
/test images
/test periodic-images
/test security
/test unit
/test verify

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-hive-master-coverage
pull-ci-openshift-hive-master-e2e
pull-ci-openshift-hive-master-e2e-pool
pull-ci-openshift-hive-master-images
pull-ci-openshift-hive-master-periodic-images
pull-ci-openshift-hive-master-security
pull-ci-openshift-hive-master-unit
pull-ci-openshift-hive-master-verify
Details

In response to this:

/retest hive-on-pull-request

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-sigs/prow repository.

@2uasimojo
Copy link
Copy Markdown
Member

/override "Red Hat Konflux"

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 17, 2025

@2uasimojo: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Red Hat Konflux

Only the following failed contexts/checkruns were expected:

  • Red Hat Konflux / hive-on-pull-request
  • ci/prow/coverage
  • ci/prow/e2e
  • ci/prow/e2e-pool
  • ci/prow/images
  • ci/prow/periodic-images
  • ci/prow/security
  • ci/prow/unit
  • ci/prow/verify
  • pull-ci-openshift-hive-master-coverage
  • pull-ci-openshift-hive-master-e2e
  • pull-ci-openshift-hive-master-e2e-pool
  • pull-ci-openshift-hive-master-images
  • pull-ci-openshift-hive-master-periodic-images
  • pull-ci-openshift-hive-master-security
  • pull-ci-openshift-hive-master-unit
  • pull-ci-openshift-hive-master-verify
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

Details

In response to this:

/override "Red Hat Konflux"

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-sigs/prow repository.

@2uasimojo 2uasimojo merged commit e9ad0fa into openshift:master Jun 17, 2025
10 of 13 checks passed
@AlexVulaj AlexVulaj deleted the HIVE-2871_always-show-upgradeable-annotation branch June 18, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants