Skip to content

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 8, 2023

For openshift/enhancements#1373

Part 3 of a multi-step plan to make individual featuregates observable via the API. This was previously avoided because when featuregates get promoted from TechPreview to Default, during an upgrade, an old operator may try to enable a TechPreview variant of a feature.

This can be addressed by keying the featuregates by version. Let's see how ugly that will be in practice now that we have a decent build-out of library-go.

Unit tests are still owed. Proof is still owed. cluster-config-operator controller is still owed.

/hold

This is proving the concept, not to be merged until it shows as working end-to-end.

Other steps include

  1. demonstrate usage in a non-bootstrap operator. probably something like openshift-apiserver or authentication
  2. enhancements write up describing how it works end to end.
  3. need to render out a version of FeatureGates that includes status for the featuregates to use during bootstrapping and plumb that through the installer.

/assign @JoelSpeed

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 8, 2023
@openshift-ci openshift-ci bot requested review from soltysh and tkashem March 8, 2023 21:18
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2023
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

No feedback on initial pass, will do another review once we have a demo end to end

@deads2k deads2k force-pushed the feature-gates branch 3 times, most recently from c803868 to 031b5b9 Compare March 15, 2023 13:12
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2023
@deads2k deads2k changed the title [wip] add featuregate status add featuregate status Mar 28, 2023
@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 Mar 28, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2023
@deads2k
Copy link
Contributor Author

deads2k commented Mar 28, 2023

now with unit tests.

launch openshift/cluster-config-operator#288,openshift/cluster-kube-apiserver-operator#1459 gcp started a cluster.

That's enough to get started. library-go and this need to merge in quick succession. The bump in kas-o can happen a bit later. The installer change can come latest. At worst, I can make that one vendor openshift/api if I get no response.

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

One nit otherwise I think this is fine, tests seem to be comprehensive.

One quick note, a point of friction that will likely come up is that people will have to agree the API change and then make sure CCO is revendored with the appropriate change. I expect the map in configv1 has sufficient consumers that it would be a pain to move the source of truth here, but, we should ensure the enhancement records why there is a split between the two repos so we have something to point people to

Comment on lines 75 to 77
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to dislike naked err returns and insist on fmt.Errorf("<context>: %w", err) to make things easier to debug down the line, up to you if you want to fix those throughout this controller or not, I expect you'll be the first one debugging errors

@deads2k
Copy link
Contributor Author

deads2k commented Apr 24, 2023

/hold cancel

this is the next step in line. Make the API available to consumers

@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 Apr 24, 2023
@JoelSpeed
Copy link
Contributor

/lgtm

Discussed in slack, future todo to move to configv1.FeatureGateName rather than lots of string casting, but not an issue now as this should be internally scoped to this controller

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@deads2k deads2k added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 24, 2023
@deads2k
Copy link
Contributor Author

deads2k commented Apr 24, 2023

adding valid bug because this is needed to unstick the cloud contorller manager bug

@deads2k
Copy link
Contributor Author

deads2k commented Apr 24, 2023

@deads2k deads2k merged commit 4f3192b into openshift:master Apr 24, 2023
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. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. 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