-
Notifications
You must be signed in to change notification settings - Fork 215
Set OKD as default featureset for OKD clusters #1287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds optional build tags support, enables SCOS (OKD) detection via a build tag, updates feature-gate initialization to migrate Default→OKD on SCOS, and bumps two OpenShift dependency versions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Prashanth684 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (51)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/config/v1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_feature.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_image_policy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_scheduling.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clusterimagepolicies.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_featuregates.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_imagepolicies.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_nodes-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_schedulers-SelfManagedHA-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/custom.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gathererconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/gatherers.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/insightsdatagatherspec.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeclaimreference.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/persistentvolumeconfig.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1/storage.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/internal/internal.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_config_client.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/generated_expansion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/config/v1/interface.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/informers/externalversions/generic.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/expansion_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/client-go/config/listers/config/v1/insightsdatagather.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (7)
Dockerfile.rhelgo.modhack/build-go.shpkg/start/start.gopkg/start/start_integration_test.gopkg/version/scos.gopkg/version/version.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/version/version.gopkg/version/scos.gogo.modDockerfile.rhelpkg/start/start_integration_test.gohack/build-go.shpkg/start/start.go
🧬 Code graph analysis (2)
pkg/version/scos.go (1)
pkg/version/version.go (1)
SCOS(23-23)
pkg/start/start.go (3)
lib/resourcebuilder/interface.go (1)
Interface(76-80)pkg/featuregates/featuregates.go (2)
CvoGates(43-52)CvoGatesFromFeatureGate(78-107)pkg/version/version.go (1)
IsSCOS(28-30)
🔇 Additional comments (15)
pkg/version/version.go (1)
20-30: LGTM!The SCOS flag and
IsSCOS()function provide a clean runtime interface for detecting OKD builds. The implementation is straightforward and follows Go conventions for build-tag-controlled behavior.Dockerfile.rhel (1)
4-5: LGTM!The
TAGSbuild argument is properly defined and passed to the build script, enabling build-tag support for OKD/SCOS builds while maintaining backward compatibility with an empty default.pkg/version/scos.go (1)
1-7: LGTM!The build-tag-guarded init function correctly enables SCOS mode when compiled with the
scostag. This is a standard Go pattern for build-time configuration.hack/build-go.sh (3)
25-28: LGTM!The conditional
TAGS_FLAGsetup correctly handles both empty and non-emptyTAGSvalues, enabling build tag support when needed.
32-32: LGTM!Build tags are properly passed to the
cluster-version-operator-testsbinary build.
43-43: LGTM!Build tags are properly passed to the
cluster-version-operatorbinary build, ensuring consistent tag support across both binaries.go.mod (1)
13-14: Both dependency versions are valid and available.The commits for
github.com/openshift/api(bfa868a22401) andgithub.com/openshift/client-go(96a6cbc1420c) exist in their respective repositories.pkg/start/start_integration_test.go (3)
195-196: LGTM: Test correctly adapted to new function signature.The test properly creates a
configClientand passes it toprocessInitialFeatureGate, matching the updated signature inpkg/start/start.go.
337-338: LGTM: Test correctly adapted to new function signature.Consistent with the other test updates.
541-542: LGTM: Test correctly adapted to new function signature.All three integration tests now properly handle the new
configClientparameter.pkg/start/start.go (5)
20-20: LGTM: Import required for patch operations.The
typespackage is correctly imported for use withtypes.MergePatchTypein the feature gate migration logic at line 298.
50-50: LGTM: Import required for SCOS detection.The
versionpackage is correctly imported to enable SCOS/OKD build detection viaversion.IsSCOS()in the feature gate initialization logic.
193-194: LGTM: Client properly created for feature gate migration.The dedicated
configClientwith user agent "feature-gate-migration" is appropriately created and passed to the updated function signature.
248-248: LGTM: Function signature correctly extended.The addition of
configClient clientset.Interfaceparameter enables the feature gate migration logic while maintaining backward compatibility with existing behavior.
272-279: LGTM: Appropriate default for SCOS builds.When the FeatureGate resource is not found, SCOS builds correctly default to the OKD feature set, while non-SCOS builds maintain the existing default behavior. The logging clearly indicates which path was taken.
|
/retest |
Add 'scos' build tag support to enable OKD-specific behavior: - Use OKD featureset as default when FeatureGate is not found - Migrate existing clusters from Default to OKD featureset on upgrade
|
@Prashanth684: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
Add 'scos' build tag support to enable OKD-specific behavior:
ref: openshift/enhancements#1899