config/v1/types_cluster_version: Add insights #1212

rluders · 2022-06-28T14:09:59Z

Add insights operator as a capability.

openshift-ci · 2022-06-28T14:10:06Z

Hello @rluders! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

config/v1/types_cluster_version.go

bparees · 2022-07-07T13:02:16Z

Do you have the other PRs created that tag these resources appropriately? Can you link them here? If they haven't already merged, it would be best to merge them first so that once this merges and gets vendored, the new capabilities values can be used successfully.

config/v1/types_cluster_version.go

rluders · 2022-07-12T12:12:22Z

Do you have the other PRs created that tag these resources appropriately? Can you link them here? If they haven't already merged, it would be best to merge them first so that once this merges and gets vendored, the new capabilities values can be used successfully.

@bparees do you mean this one? openshift/insights-operator#646

bparees · 2022-07-12T13:14:18Z

@bparees do you mean this one? openshift/insights-operator#646

yes, thanks.

fyi we've created some guidance for how to order the PRs related to capabilities changes:

https://github.com/openshift/enhancements/blob/master/enhancements/installer/component-selection.md#how-to-implement-a-new-capability

(my suggestion to merge your resource annotation PR first was actually incorrect because the CVO will start excluding those resources immediately if you did so. The new cap name has to be defined+vendored into the CVO repo first)

config/v1/types_cluster_version.go

rluders · 2022-07-22T11:41:56Z

/test verify

tremes · 2022-07-22T11:45:13Z

Who is supposed to provide no-FF labels here please? Insighs QE & docs or some other team?

rluders · 2022-07-25T07:15:41Z

@wking @bparees do you folks have any idea why its failing on verify step? Am I missing something here?

bparees · 2022-07-25T14:13:50Z

failing on a diff between what you are providing and the generated crd yaml:

diff -Naup './config/v1/0000_00_cluster-version-operator_01_clusterversion.crd.yaml' '/tmp/tmp.JlVn3mmscr/0000_00_cluster-version-operator_01_clusterversion.crd.yaml'
--- ./config/v1/0000_00_cluster-version-operator_01_clusterversion.crd.yaml	2022-07-22 12:00:23.000000000 +0000
+++ /tmp/tmp.JlVn3mmscr/0000_00_cluster-version-operator_01_clusterversion.crd.yaml	2022-07-22 12:04:19.253897103 +0000
@@ -74,6 +74,7 @@ spec:
                       enum:
                         - None
                         - v4.11
+                        - v4.12
                         - vCurrent
                 channel:
                   description: channel is an identifier for explicitly requesting that a non-default set of updates be applied to this cluster. The default channel will be contain stable updates that are appropriate for production clusters.
make: *** [Makefile:30: verify-codegen-crds-config] Error 1

do a make update, if you haven't

bparees · 2022-07-25T14:14:05Z

(and make sure you're rebased on latest master first)

wking · 2022-07-26T20:14:18Z

Who is supposed to provide no-FF labels here please?

Turns out anyone can do that. Although with out renewed focus on shipping features sprintly, it may be best to try to loop in the relevant teams for each label, just so they're all aware and lined up ready to doc/QE/etc. the new feature as the pull requests following this one bring it into 4.12 nightlies.

xJustin · 2022-07-27T14:10:07Z

Opened a docs issue in CCXDEV: https://issues.redhat.com/browse/CCXDEV-8766

/label docs-approved

JoaoFula · 2022-07-27T17:35:35Z

/label qe-approved

JoelSpeed · 2022-07-28T14:34:22Z

config/v1/types_cluster_version.go


 // ClusterVersionCapability enumerates optional, core cluster components.
-// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace
+// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;insights


@deads2k When an enum like this is using the wrong casing, what are your thoughts on updating it to allow these existing values but making new values PascalCase to fit the expected Kube conventions?

Something like this maybe with a developer note as to why some are duplicated. Consumers would have to be updated to observe both old and new values

Suggested change

// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;insights

// ---

// + Old lowercase values left for compatibility, new values should be added as PascalCase

// +kubebuilder:validation:Enum=OpenShiftSamples;Baremetal;MarketPlace;Insights;openshift-samples;baremetal;marketplace

The sins of the past. I am in favor of addressing this and the proposed solution is reasonable, but it will require a backport and an explanation (probably better a PR) of how to the CVO will handle it and backport. I'd like to work out and merge the CVO handling before adding the toleration here.

If @wking is amenable to making the CVO ToLower the value, then merging this one as "Insights" makes sense. If he's not, then I would release this one as "insights" and try to convince him before making the change.

Renamed it to Insights

deads2k · 2022-08-02T13:21:28Z

/approve

Add insights operator as a capability config/v1/types_cluster_version: version cap 4.12 - Adds the version capability set 4.12 - Moves insights cabability to version 4.12 config/v1/types_cluster_version: adds insights cap to current add capability set to cvo config crd update cvo cr manifest rename insights to Insights

openshift-ci · 2022-08-02T13:52:39Z

@rluders: 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.

sferich888 · 2022-08-02T13:54:18Z

Is this connected to https://issues.redhat.com/browse/CCXDEV-8079?

sferich888 · 2022-08-02T13:56:43Z

/label px-approved

JoelSpeed · 2022-08-02T14:01:09Z

/lgtm

openshift-ci · 2022-08-02T14:02:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, rluders

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:

~~OWNERS~~ [deads2k]

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

openshift-ci bot requested review from bparees and jwforres June 28, 2022 14:10

bparees reviewed Jul 7, 2022

View reviewed changes

config/v1/types_cluster_version.go Show resolved Hide resolved

wking mentioned this pull request Jul 7, 2022

WIP: chore(manifests): adding insights capability annotations openshift/insights-operator#651

Closed

wking reviewed Jul 7, 2022

View reviewed changes

config/v1/types_cluster_version.go Outdated Show resolved Hide resolved

rluders requested review from bparees and wking July 12, 2022 12:30

wking reviewed Jul 13, 2022

View reviewed changes

config/v1/types_cluster_version.go Outdated Show resolved Hide resolved

rluders force-pushed the io-capability branch from 36d901a to 3778341 Compare July 22, 2022 11:57

rluders mentioned this pull request Jul 22, 2022

chore(manifests): adding insights capability annotations openshift/insights-operator#646

Merged

4 tasks

rluders force-pushed the io-capability branch from 3778341 to 6acec92 Compare July 25, 2022 14:42

rluders requested a review from wking July 25, 2022 15:49

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 27, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 27, 2022

JoelSpeed reviewed Jul 28, 2022

View reviewed changes

rluders requested review from JoelSpeed and deads2k August 1, 2022 17:34

rluders force-pushed the io-capability branch from 8041ea7 to 2af53c9 Compare August 1, 2022 17:52

rluders force-pushed the io-capability branch from 2af53c9 to 4bb3610 Compare August 2, 2022 07:00

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022

rluders force-pushed the io-capability branch from 4bb3610 to 5933a6b Compare August 2, 2022 13:38

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Aug 2, 2022

openshift-ci bot assigned JoelSpeed Aug 2, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2022

openshift-merge-robot merged commit dc2694e into openshift:master Aug 2, 2022

rluders mentioned this pull request Aug 3, 2022

vendor: bump openshift/api to pick up Insights capability openshift/cluster-version-operator#812

Closed

-// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;insights
+// ---
+// + Old lowercase values left for compatibility, new values should be added as PascalCase
+// +kubebuilder:validation:Enum=OpenShiftSamples;Baremetal;MarketPlace;Insights;openshift-samples;baremetal;marketplace

config/v1/types_cluster_version: Add insights #1212

config/v1/types_cluster_version: Add insights #1212

Uh oh!

Conversation

rluders commented Jun 28, 2022

Uh oh!

openshift-ci bot commented Jun 28, 2022

Uh oh!

Uh oh!

bparees commented Jul 7, 2022

Uh oh!

Uh oh!

rluders commented Jul 12, 2022

Uh oh!

bparees commented Jul 12, 2022

Uh oh!

Uh oh!

rluders commented Jul 22, 2022

Uh oh!

tremes commented Jul 22, 2022

Uh oh!

rluders commented Jul 25, 2022

Uh oh!

bparees commented Jul 25, 2022

Uh oh!

bparees commented Jul 25, 2022

Uh oh!

wking commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xJustin commented Jul 27, 2022

Uh oh!

JoaoFula commented Jul 27, 2022

Uh oh!

JoelSpeed Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

deads2k Jul 28, 2022

Choose a reason for hiding this comment

Uh oh!

rluders Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

deads2k commented Aug 2, 2022

Uh oh!

openshift-ci bot commented Aug 2, 2022

Uh oh!

sferich888 commented Aug 2, 2022

Uh oh!

sferich888 commented Aug 2, 2022

Uh oh!

JoelSpeed commented Aug 2, 2022

Uh oh!

openshift-ci bot commented Aug 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

wking commented Jul 26, 2022 •

edited

Loading