Skip to content

Add (and use) full version label to ClusterDeployment#2084

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
2uasimojo:HIVE-2293/full-version-label
Aug 11, 2023
Merged

Add (and use) full version label to ClusterDeployment#2084
openshift-merge-robot merged 1 commit into
openshift:masterfrom
2uasimojo:HIVE-2293/full-version-label

Conversation

@2uasimojo
Copy link
Copy Markdown
Member

@2uasimojo 2uasimojo commented Aug 9, 2023

Our clusterversion controller creates labels on the ClusterDeployment based on the spoke cluster's clusterversion version object's status.desired.version string. However, previously we were using semver to parse that string and reporting only the major, minor, and patch numerals, e.g.

      "hive.openshift.io/version-major": "4",
      "hive.openshift.io/version-major-minor": "4.14",
      "hive.openshift.io/version-major-minor-patch": "4.14.0"

In cases where the clusterversion is pre-release, this lacks the pre-release information.

This commit adds a new label containing the full, un-parsed version string from clusterversion, e.g.

      "hive.openshift.io/version": "4.14.0-ec.4"

We also cut over all the internal code paths that used to use the major-minor-patch label for version checking, as the full string will always be more accurate, particularly when determining whether we're before or after a version containing specific functionality.

The other external-facing effect is in the tabular output of oc get cd, which now reports the full version string rather than the major-minor-patch string. For GA-level clusters, this will be identical; it will only differ (being more precise) for pre-release clusters.

HIVE-2293

Our clusterversion controller creates labels on the ClusterDeployment
based on the spoke cluster's `clusterversion version` object's
`status.desired.version` string. However, previously we were using
`semver` to parse that string and reporting only the major, minor, and
patch numerals, e.g.

```
      "hive.openshift.io/version-major": "4",
      "hive.openshift.io/version-major-minor": "4.14",
      "hive.openshift.io/version-major-minor-patch": "4.14.0"
```

In cases where the clusterversion is pre-release, this lacks the
pre-release information.

This commit adds a new label containing the full, un-parsed version
string from clusterversion, e.g.

```
      "hive.openshift.io/version": "4.14.0-ec.4"
```

We also cut over all the internal code paths that used to use the
major-minor-patch label for version checking, as the full string will
always be more accurate, particularly when determining whether we're
before or after a version containing specific functionality.

The other external-facing effect is in the tabular output of `oc get
cd`, which now reports the full version string rather than the
major-minor-patch string. For GA-level clusters, this will be identical;
it will only differ (being more precise) for pre-release clusters.

HIVE-2293
@openshift-ci openshift-ci Bot requested review from abutcher and suhanime August 9, 2023 21:02
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2023
@2uasimojo
Copy link
Copy Markdown
Member Author

/assign @suhanime

- jsonPath: .metadata.labels.hive\.openshift\.io/cluster-region
name: Region
type: string
- jsonPath: .metadata.labels.hive\.openshift\.io/version-major-minor-patch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we completely sure no one uses this label before we remove it? From what I understand, the practice is to keep both and let the user decide which one they want to use

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We’re not removing the label, just changing which label we use in oc get cd

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To expand a bit: 99% of clusters (and 100% of prod clusters, since they should only be using GAed releases) will have the same value here before & after.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 9, 2023

Codecov Report

Merging #2084 (ecca33b) into master (01532c6) will increase coverage by 0.13%.
Report is 12 commits behind head on master.
The diff coverage is 72.72%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2084      +/-   ##
==========================================
+ Coverage   57.38%   57.52%   +0.13%     
==========================================
  Files         186      186              
  Lines       25712    25856     +144     
==========================================
+ Hits        14756    14874     +118     
- Misses       9717     9734      +17     
- Partials     1239     1248       +9     
Files Changed Coverage Δ
pkg/constants/constants.go 100.00% <ø> (ø)
pkg/controller/metrics/metrics.go 34.21% <0.00%> (ø)
...g/controller/machinepool/machinepool_controller.go 50.06% <33.33%> (ø)
...g/controller/hibernation/hibernation_controller.go 65.00% <50.00%> (ø)
...roller/clusterversion/clusterversion_controller.go 43.43% <81.81%> (+3.00%) ⬆️
pkg/clusterresource/aws.go 90.62% <100.00%> (ø)
...g/controller/clusterpool/clusterpool_controller.go 60.02% <100.00%> (+3.72%) ⬆️
pkg/test/clusterdeployment/clusterdeployment.go 97.32% <100.00%> (ø)

@2uasimojo
Copy link
Copy Markdown
Member Author

/test e2e-pool

claim deletion hung: improving artifact gathering via #2086.

Copy link
Copy Markdown
Contributor

@suhanime suhanime 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 11, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 11, 2023

@2uasimojo: 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 3b6b0a0 into openshift:master Aug 11, 2023
@2uasimojo 2uasimojo deleted the HIVE-2293/full-version-label branch August 11, 2023 19:30
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