featuregate for catalogd metas web api by grokspawn · Pull Request #2202 · openshift/api

grokspawn · 2025-02-12T22:01:19Z

No description provided.

openshift-ci · 2025-02-12T22:01:35Z

Hello @grokspawn! 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.

JoelSpeed · 2025-02-14T10:45:14Z

features/features.go

+					contactPerson("jordank").
+					productScope(ocpSpecific).
+					enhancementPR("https://github.com/openshift/enhancements/pull/1749").
+					enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).


This is deliberately not being enabled for HyperShift clusters?

No, I think this is just where I'm uncertain about how to go about this. There are Hypershift and SelfManaged options, and I need the latter, but it's unclear how to also enable the former.

Do I list the feature twice (once for each)? Is Hypershift a superset of SelfManaged?

Without some feedback from SNO reviewers I don't think we want this landing as a surprise there, but otherwise this feature should be everywhere else.

Ooh. Having played with it a bit, it appears that Hypershift is a superset of SelfManaged, so I /think/ I've fixed it.

NewOLM is not enabled on Hypershift (yet), therefore any new subfeature of NewOLM (like this one) should also not be enabled on hypershift.

grokspawn · 2025-02-17T21:31:30Z

Didn't see anything in the CI failures that looked actionable, and was able to bring up a cluster with this PR merged, so I'm calling this 'infra' and re-kicking.

/retest

joelanford · 2025-02-17T21:57:21Z

features/features.go

 				enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade, configv1.Default).
 				mustRegister()

+	FeatureGateNewOLMCatalogdMetas = newFeatureGate("NewOLMCatalogdMetas").


Should we include APIV1 in the name in case there is ever an /api/v2/metas endpoint behind a different feature gate?

Suggested change

FeatureGateNewOLMCatalogdMetas = newFeatureGate("NewOLMCatalogdMetas").

FeatureGateNewOLMCatalogdMetas = newFeatureGate("NewOLMCatalogdAPIV1Metas").

joelanford · 2025-02-17T21:57:57Z

features/features.go

+					contactPerson("jordank").
+					productScope(ocpSpecific).
+					enhancementPR("https://github.com/openshift/enhancements/pull/1749").
+					enableForClusterProfile(Hypershift, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).


As I mentioned in another thread, NewOLM (the parent feature for OLMv1) is not yet enabled in Hypershift. This feature gate needs to align with NewOLM.

Suggested change

enableForClusterProfile(Hypershift, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

JoelSpeed · 2025-02-18T11:01:26Z

features/features.go

+					contactPerson("jordank").
+					productScope(ocpSpecific).
+					enhancementPR("https://github.com/openshift/enhancements/pull/1749").
+					enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).


I think you just want to enable everywhere and not make a distinction between cluster profiles

Suggested change

enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

That would enable us for hypershift, and the new feature flag can't be enable there because its "parent" NewOLM cannot be enabled there yet.

Ack, in which case you have it correct already!

JoelSpeed · 2025-02-18T14:49:24Z

/approve

Will let @joelanford add the LGTM if he is happy with the name

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

joelanford · 2025-02-18T22:16:39Z

/lgtm

Thanks for the review @JoelSpeed !

openshift-ci · 2025-02-18T22:17:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grokspawn, joelanford, 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:

~~OWNERS~~ [JoelSpeed]

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

grokspawn · 2025-02-19T14:44:46Z

/retest-required

openshift-ci-robot · 2025-02-20T00:30:08Z

/retest-required

Remaining retests: 0 against base HEAD ead8ee7 and 2 for PR HEAD cedb085 in total

openshift-ci · 2025-02-20T02:49:07Z

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

openshift-bot · 2025-02-20T06:10:16Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.19.0-202502200538.p0.gcfbda0b.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 12, 2025

openshift-ci bot requested review from derekwaynecarr and knobunc February 12, 2025 22:02

grokspawn force-pushed the catalogd-metasapi-feature branch from 47b1d07 to 9628bae Compare February 12, 2025 22:06

JoelSpeed reviewed Feb 14, 2025

View reviewed changes

grokspawn force-pushed the catalogd-metasapi-feature branch from 9628bae to de712d2 Compare February 14, 2025 15:58

openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2025

grokspawn force-pushed the catalogd-metasapi-feature branch from de712d2 to 5c3885a Compare February 14, 2025 16:46

joelanford reviewed Feb 17, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2025

JoelSpeed reviewed Feb 18, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2025

grokspawn added 2 commits February 18, 2025 15:24

featuregate for catalogd metas web api

d0f74a9

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

review resolution

cedb085

Signed-off-by: Jordan Keister <jordan@nimblewidget.com>

grokspawn force-pushed the catalogd-metasapi-feature branch from 9b88560 to cedb085 Compare February 18, 2025 21:35

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2025

openshift-ci bot assigned joelanford Feb 18, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2025

openshift-merge-bot bot merged commit cfbda0b into openshift:master Feb 20, 2025
22 checks passed

	FeatureGateNewOLMCatalogdMetas = newFeatureGate("NewOLMCatalogdMetas").
	FeatureGateNewOLMCatalogdMetas = newFeatureGate("NewOLMCatalogdAPIV1Metas").

	enableForClusterProfile(Hypershift, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
	enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

	enableForClusterProfile(SelfManaged, configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
	enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).

Conversation

grokspawn commented Feb 12, 2025

Uh oh!

openshift-ci bot commented Feb 12, 2025

Uh oh!

JoelSpeed Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

grokspawn Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

grokspawn Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

joelanford Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

grokspawn commented Feb 17, 2025

Uh oh!

joelanford Feb 17, 2025

Choose a reason for hiding this comment

Uh oh!

joelanford Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

grokspawn Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Feb 18, 2025

Uh oh!

joelanford commented Feb 18, 2025

Uh oh!

openshift-ci bot commented Feb 18, 2025

Uh oh!

grokspawn commented Feb 19, 2025

Uh oh!

openshift-ci-robot commented Feb 20, 2025

Uh oh!

openshift-ci bot commented Feb 20, 2025

Uh oh!

Uh oh!

openshift-bot commented Feb 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joelanford Feb 17, 2025 •

edited

Loading

grokspawn Feb 18, 2025 •

edited

Loading