OCPSTRAT-2876: Enable major version segmentation of enabled feature gates#2637
OCPSTRAT-2876: Enable major version segmentation of enabled feature gates#2637JoelSpeed wants to merge 11 commits intoopenshift:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request refactors the feature gate system to support version-aware gating. The core changes introduce a versioned evaluation model where feature gates can be enabled or disabled based on both cluster profile/feature set and OpenShift major version. The feature gate data structure is reorganized from nested maps keyed by cluster profile and feature set to a flat map keyed by gate name with version-aware statuses. Feature gate manifests are now consolidated and deduplicated across versions, with new annotations tracking applicable major versions and feature sets. A semver dependency is added for version parsing, and code generation logic is updated to handle multi-dimensional rendering across versions, cluster profiles, and feature sets. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XXL
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
payload-command/render/write_featureset.go (1)
209-245: Consider deterministic iteration order for reproducibility.Map iteration over
consolidatedGroupsis non-deterministic. While this doesn't affect correctness (each file has unique content), it may cause non-reproducible build artifacts when diffing generated outputs.🔎 Proposed fix for deterministic ordering
+ // Sort keys for deterministic output + var sortedKeys []profileFeatureSet + for key := range consolidatedGroups { + sortedKeys = append(sortedKeys, key) + } + sort.Slice(sortedKeys, func(i, j int) bool { + if sortedKeys[i].clusterProfile != sortedKeys[j].clusterProfile { + return sortedKeys[i].clusterProfile < sortedKeys[j].clusterProfile + } + return sortedKeys[i].featureSetName < sortedKeys[j].featureSetName + }) + // Generate files for each consolidated group - for groupKey, versionGroups := range consolidatedGroups { + for _, groupKey := range sortedKeys { + versionGroups := consolidatedGroups[groupKey] for _, group := range versionGroups {features/util.go (1)
89-134: Dead code:statusByClusterProfileByFeatureSetis no longer used.The field
statusByClusterProfileByFeatureSetonfeatureGateBuilder(line 98) and its initialization innewFeatureGate()(lines 124-132) appear to be legacy code that's no longer used after the refactor to the option-basedstatus []featureGateStatusapproach.🔎 Proposed cleanup
type featureGateBuilder struct { name string owningJiraComponent string responsiblePerson string owningProduct OwningProduct enhancementPRURL string status []featureGateStatus - - statusByClusterProfileByFeatureSet map[ClusterProfileName]map[configv1.FeatureSet]bool }func newFeatureGate(name string) *featureGateBuilder { b := &featureGateBuilder{ - name: name, - statusByClusterProfileByFeatureSet: map[ClusterProfileName]map[configv1.FeatureSet]bool{}, + name: name, } - for _, clusterProfile := range AllClusterProfiles { - byFeatureSet := map[configv1.FeatureSet]bool{} - for _, featureSet := range configv1.AllFixedFeatureSets { - byFeatureSet[featureSet] = false - } - b.statusByClusterProfileByFeatureSet[clusterProfile] = byFeatureSet - } return b }features/features.go (1)
81-83: Unused helper functioninCustomNoUpgrade().The
inCustomNoUpgrade()helper is defined but never used in any feature gate definitions. This may be intentional (CustomNoUpgrade is user-controlled), but if so, consider removing the unused function or adding a comment explaining its purpose.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (8)
go.sumis excluded by!**/*.sumvendor/github.com/blang/semver/v4/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/range.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/semver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/sort.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/blang/semver/v4/sql.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (15)
features/features.gofeatures/okd_featureset_parity_test.gofeatures/util.gogo.modhack/update-payload-featuregates.shpayload-command/render/render.gopayload-command/render/write_featureset.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
🧰 Additional context used
🧬 Code graph analysis (4)
features/okd_featureset_parity_test.go (2)
config/v1/types_feature.go (3)
Default(41-41)OKD(58-58)FeatureGateAttributes(136-144)insights/v1alpha1/types_insights.go (1)
Enabled(133-133)
features/features.go (2)
features/util.go (5)
ClusterProfileName(34-34)FeatureGateDescription(13-27)AllClusterProfiles(39-39)SelfManaged(38-38)Hypershift(37-37)config/v1/types_feature.go (3)
FeatureSet(37-37)FeatureGateAttributes(136-144)AllFixedFeatureSets(61-61)
features/util.go (1)
config/v1/types_feature.go (6)
FeatureSet(37-37)Default(41-41)TechPreviewNoUpgrade(45-45)DevPreviewNoUpgrade(49-49)CustomNoUpgrade(54-54)OKD(58-58)
payload-command/render/render.go (3)
features/util.go (1)
ClusterProfileName(34-34)features/features.go (1)
FeatureSets(8-37)config/v1/types_feature.go (3)
FeatureSet(37-37)FeatureGate(21-35)Default(41-41)
🔇 Additional comments (22)
hack/update-payload-featuregates.sh (1)
5-5: LGTM - Cleanup step ensures deterministic regeneration.The pre-generation cleanup prevents stale manifests from persisting when feature gates are removed or renamed. This is essential now that version-based gating may produce different file sets.
payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml (1)
6-7: Generated manifest includes new major-version annotation.The addition of
release.openshift.io/major-version: "4,5,6,7,8,9,10"aligns with the PR's goal of enabling version-based feature gating. The broad range (4-10) ensures this manifest applies across all supported major versions.payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml (1)
6-7: Consistent major-version annotation for Hypershift profile.The annotation follows the same pattern as other manifests, maintaining consistency across cluster profiles.
payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for TechPreviewNoUpgrade feature set.payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for DevPreviewNoUpgrade feature set.payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml (1)
6-7: LGTM - Consistent annotation for Hypershift DevPreviewNoUpgrade.go.mod (1)
6-6: New semver dependency for version parsing.The
github.com/blang/semver/v4package is a well-established choice for semantic version parsing in Go. This dependency is actively used inpayload-command/render/render.goto parse payload versions viasemver.ParseTolerant(), enabling the version-aware feature gate computation introduced in this PR.features/okd_featureset_parity_test.go (1)
19-53: LGTM! Test correctly adapted to version-aware data structure.The test has been properly updated to handle the new nested structure returned by
AllFeatureSets(), adding an outer iteration level while preserving the core validation logic that ensures OKD includes all Default feature gates. The error reporting is enhanced with cluster profile context.payload-command/render/render.go (3)
73-76: Version parsing looks correct.Using
semver.ParseTolerantis appropriate for handling various payload version formats. The error handling ensures parsing failures are caught early.
94-94: Major version propagation implemented correctly.The payload major version is consistently extracted from the parsed semver and passed to all
FeatureSetscalls. The function signature update maintains the originalpayloadVersionstring parameter alongside the newpayloadMajorVersionparameter, ensuring both are available where needed.Also applies to: 115-115, 140-140, 169-169
11-11: No action needed — the semver library is at stable version v4.0.0 with no known security vulnerabilities.payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml (1)
6-7: This version range is consistent across all feature gate manifests in the repository and appears to be intentional rather than a placeholder. All 8 feature gate manifests use the identical annotation value "4,5,6,7,8,9,10", indicating this is a deliberate project-wide decision, likely for forward compatibility.payload-command/render/write_featureset.go (4)
24-46: LGTM!The
versionGroupstruct and its methods are well-designed. TheversionRangeString()handles single and range versions cleanly, andversions()correctly iterates the inclusive range.
48-72: LGTM!The equality comparison using sets is a clean approach. Both nil checks and the set-based equality for Enabled/Disabled slices are correctly implemented.
124-133: LGTM!The filename generation logic is clear. The fallback to "Default" for empty feature set names is appropriate.
168-187: LGTM!The legacy feature gate collection correctly traverses the new version-keyed structure and maintains the existing detection logic for gates predating 4.18.
features/util.go (3)
49-87: LGTM!The functional options pattern is cleanly implemented. The convenience functions like
inDefault(),inTechPreviewNoUpgrade(), etc., provide a readable API for feature gate configuration.
100-114: LGTM!The
isEnabledmethod correctly implements permissive matching for version and cluster profile (empty sets match all), while requiring explicit feature set membership. This design enables flexible gate configuration.
156-170: LGTM!The
enable()method correctly accumulates status entries, allowing for flexible OR-based gating logic across multiple calls.features/features.go (3)
8-37: LGTM!The
FeatureSetsfunction correctly evaluates each gate's status using OR semantics (enabled if any status matches) and properly classifies gates as enabled or disabled.
39-64: LGTM!The version collection logic correctly combines hardcoded future versions with explicitly configured ones. The nested map construction is clear. The comment about future-proofing until 2040 is helpful context.
66-70: LGTM!The updated
allFeatureGatesdeclaration and the consistent use of the newenable()API across all feature gate definitions is well-structured.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A handful of high-level comments I had from a brief look at this PR.
Will wait for responses related to these comments before digging further into the PR.
features/util.go
Outdated
|
|
||
| status []featureGateStatus | ||
|
|
||
| statusByClusterProfileByFeatureSet map[ClusterProfileName]map[configv1.FeatureSet]bool |
There was a problem hiding this comment.
Would we still need this field or does it become effectively unused?
There was a problem hiding this comment.
Ahh yep, that's unused, will cleanup
There was a problem hiding this comment.
Looks like this still needs cleanup?
| Name: name, | ||
| }, | ||
| }) | ||
| } else { |
There was a problem hiding this comment.
Non-blocking nit: continue in above if enabled block instead of an else follows standard early return control flows.
JoelSpeed
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: This pull request references OCPSTRAT-2876 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
JoelSpeed
force-pushed
the
major-version-feature-gating
branch
from
5c2c76c to
c2a6a81
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tools/codegen/pkg/manifestmerge/filters.go`:
- Around line 569-598: The equality check in VersionFilter.UseManifest is too
strict; instead of requiring exact set equality between f.targetVersion and the
parsed manifest versions (versionRange), change the logic to include the
manifest when the two sets intersect non-empty. In UseManifest, after creating
versionRange and manifestSet (sets.New[uint64](versionRange...)), return true
when f.targetVersion.Intersection(manifestSet).Len() > 0 (or equivalent
HasAny/ContainsAny if available) and false otherwise, preserving the same error
handling and using partialObject/GetName for context.
- Around line 296-300: The code uses sets.NewInt() to collect versions from
allVersioned which are uint64, risking truncation on 32-bit platforms; change
versionSet := sets.NewInt() to versionSet := sets.NewUint64() and stop casting
the loop values to int—use versionSet.Insert(version) (or cast to uint64 if the
element type differs) when iterating vfs.VersionRange so the uint64 versions are
stored accurately; update any downstream uses of versionSet accordingly.
In `@tools/codegen/pkg/manifestmerge/generator.go`:
- Around line 544-546: The branch handling case allClusterProfilesSame is
accidentally shadowing the outer nameComponents by using := inside the loop over
crd.featureSet.UnsortedList(); change the loop append from using := to using =
(i.e., nameComponents = append(nameComponents, featureSet)) so the outer
nameComponents (which includes the version component) is preserved and the
filename assembly uses the version as intended.
- Around line 564-565: The code inside the loop over
crd.featureSet.UnsortedList() declares a new shadowed variable with
"nameComponents := append(...)" which prevents the outer nameComponents (which
contains version info) from being used in the filename; change the declaration
to avoid shadowing—either assign back to the existing slice (nameComponents =
append(nameComponents, clusterProfileShortName, featureSet)) if you intend to
mutate, or better create a fresh slice (e.g., newNameComponents :=
append(append([]string{}, nameComponents...), clusterProfileShortName,
featureSet)) so the outer nameComponents remains intact; update occurrences
accordingly where the filename is built (look for nameComponents,
clusterProfileShortName, featureSet in generator.go).
🧹 Nitpick comments (3)
features/features.go (1)
39-64: Consider making the hardcoded version list configurable or documenting the assumption.The hardcoded versions
4, 5, 6, 7, 8, 9, 10with the comment "future proofs us until at least 2040" is reasonable for now, but this could become a maintenance concern. The dynamic discovery of versions from gate definitions (lines 46-50) is a good fallback.tools/codegen/pkg/manifestmerge/filters.go (1)
246-252: Silent error handling may hide issues.When
parseFeatureGateFilefails, the error is silently ignored. While this provides backward compatibility, it may mask legitimate parsing errors. Consider logging these errors at debug level for troubleshooting.tools/codegen/pkg/manifestmerge/generator.go (1)
186-200: Silently continuing onFilterForVersionedFeatureSeterrors may hide issues.When
FilterForVersionedFeatureSetreturns an error (lines 196-200), the code silently continues to the next combination. While this handles missing version/profile/featureset combinations gracefully, legitimate errors could be hidden.💡 Consider distinguishing "not found" from actual errors
The
FilterForVersionedFeatureSetfunction returns a generic error. Consider having it return a sentinel error (likeErrNoFeatureGateFile) for "not found" cases, allowing the caller to distinguish expected gaps from unexpected failures.
| versionSet := sets.NewInt() | ||
| for _, vfs := range allVersioned { | ||
| for _, version := range vfs.VersionRange { | ||
| versionSet.Insert(int(version)) | ||
| } |
There was a problem hiding this comment.
Type mismatch: sets.NewInt() truncates uint64 versions.
Using sets.NewInt() (which uses int) to store uint64 versions could truncate values on 32-bit systems. While major versions are unlikely to exceed int32 range, this is technically incorrect.
🔧 Proposed fix using uint64 set
- versionSet := sets.NewInt()
+ versionSet := sets.New[uint64]()
for _, vfs := range allVersioned {
for _, version := range vfs.VersionRange {
- versionSet.Insert(int(version))
+ versionSet.Insert(version)
}
}
- versions := make([]uint64, 0, versionSet.Len())
- for _, v := range versionSet.UnsortedList() {
- versions = append(versions, uint64(v))
- }
+ versions := versionSet.UnsortedList()🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 296 - 300, The code
uses sets.NewInt() to collect versions from allVersioned which are uint64,
risking truncation on 32-bit platforms; change versionSet := sets.NewInt() to
versionSet := sets.NewUint64() and stop casting the loop values to int—use
versionSet.Insert(version) (or cast to uint64 if the element type differs) when
iterating vfs.VersionRange so the uint64 versions are stored accurately; update
any downstream uses of versionSet accordingly.
| type VersionFilter struct { | ||
| targetVersion sets.Set[uint64] | ||
| } | ||
|
|
||
| func (f *VersionFilter) UseManifest(data []byte) (bool, error) { | ||
| partialObject := &metav1.PartialObjectMetadata{} | ||
| if err := kyaml.Unmarshal(data, partialObject); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| versionStr, found := partialObject.GetAnnotations()["release.openshift.io/major-version"] | ||
| if !found { | ||
| // If the manifest doesn't restrict itself to a specific version, we must include it. | ||
| return true, nil | ||
| } | ||
|
|
||
| versionRange, err := parseVersionsFromAnnotation(versionStr) | ||
| if err != nil { | ||
| return false, fmt.Errorf("invalid version annotation in %s: %w", partialObject.GetName(), err) | ||
| } | ||
| return f.targetVersion.Equal(sets.New[uint64](versionRange...)), nil | ||
| } | ||
|
|
||
| func (f *VersionFilter) UseCRD(metadata crdForFeatureSet) bool { | ||
| return f.targetVersion.Equal(metadata.version) | ||
| } | ||
|
|
||
| func (f *VersionFilter) String() string { | ||
| return fmt.Sprintf("version=%d", f.targetVersion) | ||
| } |
There was a problem hiding this comment.
VersionFilter.UseManifest uses exact set equality which may be too strict.
Line 589 uses f.targetVersion.Equal(sets.New[uint64](versionRange...)), requiring the manifest's version range to exactly match the filter's target version set. This seems overly strict — typically you'd want to check if the manifest applies to any of the target versions.
🐛 Proposed fix for intersection-based matching
- return f.targetVersion.Equal(sets.New[uint64](versionRange...)), nil
+ // Check if any version in the manifest's range matches any target version
+ manifestVersions := sets.New[uint64](versionRange...)
+ return f.targetVersion.Intersection(manifestVersions).Len() > 0, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type VersionFilter struct { | |
| targetVersion sets.Set[uint64] | |
| } | |
| func (f *VersionFilter) UseManifest(data []byte) (bool, error) { | |
| partialObject := &metav1.PartialObjectMetadata{} | |
| if err := kyaml.Unmarshal(data, partialObject); err != nil { | |
| return false, err | |
| } | |
| versionStr, found := partialObject.GetAnnotations()["release.openshift.io/major-version"] | |
| if !found { | |
| // If the manifest doesn't restrict itself to a specific version, we must include it. | |
| return true, nil | |
| } | |
| versionRange, err := parseVersionsFromAnnotation(versionStr) | |
| if err != nil { | |
| return false, fmt.Errorf("invalid version annotation in %s: %w", partialObject.GetName(), err) | |
| } | |
| return f.targetVersion.Equal(sets.New[uint64](versionRange...)), nil | |
| } | |
| func (f *VersionFilter) UseCRD(metadata crdForFeatureSet) bool { | |
| return f.targetVersion.Equal(metadata.version) | |
| } | |
| func (f *VersionFilter) String() string { | |
| return fmt.Sprintf("version=%d", f.targetVersion) | |
| } | |
| type VersionFilter struct { | |
| targetVersion sets.Set[uint64] | |
| } | |
| func (f *VersionFilter) UseManifest(data []byte) (bool, error) { | |
| partialObject := &metav1.PartialObjectMetadata{} | |
| if err := kyaml.Unmarshal(data, partialObject); err != nil { | |
| return false, err | |
| } | |
| versionStr, found := partialObject.GetAnnotations()["release.openshift.io/major-version"] | |
| if !found { | |
| // If the manifest doesn't restrict itself to a specific version, we must include it. | |
| return true, nil | |
| } | |
| versionRange, err := parseVersionsFromAnnotation(versionStr) | |
| if err != nil { | |
| return false, fmt.Errorf("invalid version annotation in %s: %w", partialObject.GetName(), err) | |
| } | |
| // Check if any version in the manifest's range matches any target version | |
| manifestVersions := sets.New[uint64](versionRange...) | |
| return f.targetVersion.Intersection(manifestVersions).Len() > 0, nil | |
| } | |
| func (f *VersionFilter) UseCRD(metadata crdForFeatureSet) bool { | |
| return f.targetVersion.Equal(metadata.version) | |
| } | |
| func (f *VersionFilter) String() string { | |
| return fmt.Sprintf("version=%d", f.targetVersion) | |
| } |
🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 569 - 598, The
equality check in VersionFilter.UseManifest is too strict; instead of requiring
exact set equality between f.targetVersion and the parsed manifest versions
(versionRange), change the logic to include the manifest when the two sets
intersect non-empty. In UseManifest, after creating versionRange and manifestSet
(sets.New[uint64](versionRange...)), return true when
f.targetVersion.Intersection(manifestSet).Len() > 0 (or equivalent
HasAny/ContainsAny if available) and false otherwise, preserving the same error
handling and using partialObject/GetName for context.
| case allClusterProfilesSame: | ||
| for _, featureSet := range crd.featureSet.UnsortedList() { | ||
| nameComponents := append(nameComponents, featureSet) |
There was a problem hiding this comment.
Bug: nameComponents variable shadowing in loop.
Line 546 uses := which shadows the outer nameComponents variable declared at line 500. This means the version component (if any) from line 506 won't be included in the filename for this branch.
🐛 Proposed fix
case allClusterProfilesSame:
for _, featureSet := range crd.featureSet.UnsortedList() {
- nameComponents := append(nameComponents, featureSet)
+ featureSetComponents := append(slices.Clone(nameComponents), featureSet)
- crdFilename := getCRDFilename(crdFilenamePattern, nameComponents)
+ crdFilename := getCRDFilename(crdFilenamePattern, featureSetComponents)🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/generator.go` around lines 544 - 546, The
branch handling case allClusterProfilesSame is accidentally shadowing the outer
nameComponents by using := inside the loop over crd.featureSet.UnsortedList();
change the loop append from using := to using = (i.e., nameComponents =
append(nameComponents, featureSet)) so the outer nameComponents (which includes
the version component) is preserved and the filename assembly uses the version
as intended.
c2a6a81 to
32dd9b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@payload-command/render/write_featureset.go`:
- Around line 48-72: The featureGateContentEqual function compares
FeatureGateDescription structs using sets.New(a.Enabled...) which treats
differing metadata as inequality; change the comparison to use only gate names
by building sets of names (e.g., iterate a.Enabled and a.Disabled to collect
each FeatureGateDescription.Name into sets) and compare those name-sets instead
(update references inside featureGateContentEqual to use name-based sets for
aEnabledNames, bEnabledNames, aDisabledNames, bDisabledNames).
In `@tools/codegen/pkg/manifestmerge/filters.go`:
- Around line 246-252: The loop in AllVersionedFeatureSets currently swallows
every error from parseFeatureGateFile when iterating files (filePath built from
payloadFeatureGatePath and currFeatureSetManifestFile.Name()), which can hide
real parse/read failures; change the error handling so you detect and skip only
the "not-versioned" sentinel (or a dedicated ErrNotVersioned) returned by
parseFeatureGateFile, but for any other error return it (or log/propagate it)
instead of continuing, ensuring malformed YAML or IO errors surface to the
caller.
- Around line 596-597: The VersionFilter.String method currently formats
f.targetVersion as an integer; update the formatting to print the set's contents
by using f.targetVersion.UnsortedList() with a %v verb so the
VersionFilter.String (function VersionFilter.String) produces a correct,
human-readable representation of the sets.Set[uint64] stored in targetVersion.
- Around line 163-175: The hardcoded switch in parseFeatureGateFile should be
removed and the authoritative mapping in utils.ClusterProfileToShortName used
instead: when you find an annotation with value
"false-except-for-the-config-operator", derive a canonical profile key by
trimming the common prefix (e.g. strings.TrimPrefix(annotationKey,
"include.release.openshift.io/")) and call
utils.ClusterProfileToShortName(profileKey) to get clusterProfile; assign that
result to clusterProfile and handle any error/unrecognized-profile return from
ClusterProfileToShortName rather than falling back to the raw annotation key.
In `@tools/codegen/pkg/manifestmerge/generator.go`:
- Around line 526-543: The loop mutates the shared nameComponents slice causing
filenames to accumulate values across iterations; fix by making a per-iteration
copy of nameComponents (e.g., nameComponentsCopy := append([]string{},
nameComponents...)) and append clusterProfileShortName to that copy, then use
nameComponentsCopy when calling getCRDFilename(crdFilenamePattern,
nameComponentsCopy) and when building the crdForFeatureSet entry (keep other
fields like crd.crd.DeepCopy(), clusterProfile, version, outputFile unchanged).
🧹 Nitpick comments (2)
tools/codegen/pkg/manifestmerge/generator.go (1)
602-608: Non-deterministic output: feature set annotation order is unstable.
crd.featureSet.UnsortedList()returns elements in non-deterministic order. The resulting annotation value will vary between runs, causing unnecessary diffs in generated files.♻️ Proposed fix to sort feature sets
if len(crdsToWrite[i].featureSet) > 0 { featureSetStrings := []string{} for _, featureSet := range crdsToWrite[i].featureSet.UnsortedList() { featureSetStrings = append(featureSetStrings, featureSet) } + slices.Sort(featureSetStrings) annotations["release.openshift.io/feature-set"] = strings.Join(featureSetStrings, ",") }features/features.go (1)
8-10: Document version range rationale.The comment mentions "future proofs us until at least 2040" but doesn't explain the basis for min=4 and max=10. Consider adding a brief note about why these specific bounds were chosen (e.g., current major version, expected release cadence).
| // Determine cluster profile from the annotations | ||
| var clusterProfile string | ||
| for annotationKey, annotationValue := range annotations { | ||
| if annotationValue == "false-except-for-the-config-operator" { | ||
| // Convert long cluster profile names to short names | ||
| switch annotationKey { | ||
| case "include.release.openshift.io/ibm-cloud-managed": | ||
| clusterProfile = "Hypershift" | ||
| case "include.release.openshift.io/self-managed-high-availability": | ||
| clusterProfile = "SelfManagedHA" | ||
| default: | ||
| clusterProfile = annotationKey // fallback to the key itself | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and examine the context around lines 163-175
head -n 180 tools/codegen/pkg/manifestmerge/filters.go | tail -n 25Repository: openshift/api
Length of output: 869
🏁 Script executed:
# Find the utils.ClusterProfileToShortName function definition
rg "func ClusterProfileToShortName" -A 20 --type=goRepository: openshift/api
Length of output: 1110
🏁 Script executed:
# Search for how cluster profiles are used and matched elsewhere
rg "ClusterProfileToShortName" --type=go -B 2 -A 2Repository: openshift/api
Length of output: 2661
🏁 Script executed:
# Look for parseFeatureGateFile function to understand data flow
rg "parseFeatureGateFile" --type=go -B 2 -A 5Repository: openshift/api
Length of output: 1567
🏁 Script executed:
# Search for any other references to the annotation keys pattern
rg "include.release.openshift.io" --type=go -nRepository: openshift/api
Length of output: 4483
🏁 Script executed:
# Get the full map definition in featureset.go
rg "clusterProfileToShortName\s*=" -A 10 --type=go tools/codegen/pkg/utils/featureset.goRepository: openshift/api
Length of output: 458
🏁 Script executed:
# Check test file to see what profiles are expected
cat -n tests/crd_filter.go | head -n 25Repository: openshift/api
Length of output: 861
🏁 Script executed:
# Search for actual usage of single-node-developer in the codebase
rg "single-node-developer" --type=goRepository: openshift/api
Length of output: 799
🏁 Script executed:
# Verify the error handling when utils.ClusterProfileToShortName fails in filters.go
rg -A 5 "utils.ClusterProfileToShortName\(clusterProfile\)" tools/codegen/pkg/manifestmerge/filters.goRepository: openshift/api
Length of output: 583
🏁 Script executed:
# Check if the test map is used or if it's just a test fixture
rg "clusterProfileToShortName" tests/crd_filter.go -B 5 -A 5Repository: openshift/api
Length of output: 1002
Hardcoded cluster-profile switch statement will fail when new profiles are added to the authoritative mapping.
The parseFeatureGateFile function uses a hardcoded switch statement (lines 169-173) that only handles 2 cluster profiles. When new profiles are added to utils.ClusterProfileToShortName(), this will cause downstream errors because the code later calls utils.ClusterProfileToShortName(clusterProfile) which will fail with an "unrecognized clusterprofile name" error.
Instead of the proposed diff (which incorrectly strips the prefix before calling the utility function), normalize annotation keys by using the authoritative mapping directly. Consider:
- Centralizing the cluster profile mapping to a single source of truth (currently split between
utils/featureset.goand test fixtures) - Using a consistent approach throughout the codebase instead of hardcoding special cases
- Handling new profiles dynamically rather than requiring code changes
Note: The test file tests/crd_filter.go already includes "single-node-developer" in its map, suggesting it's a supported profile that's missing from the main utils/featureset.go mapping.
🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 163 - 175, The
hardcoded switch in parseFeatureGateFile should be removed and the authoritative
mapping in utils.ClusterProfileToShortName used instead: when you find an
annotation with value "false-except-for-the-config-operator", derive a canonical
profile key by trimming the common prefix (e.g.
strings.TrimPrefix(annotationKey, "include.release.openshift.io/")) and call
utils.ClusterProfileToShortName(profileKey) to get clusterProfile; assign that
result to clusterProfile and handle any error/unrecognized-profile return from
ClusterProfileToShortName rather than falling back to the raw annotation key.
There was a problem hiding this comment.
I don't disagree that a centralized definition for the translations here would be nice, but I wouldn't block on this.
There was a problem hiding this comment.
@everettraven, understood! I'll defer to your judgment on this. The current implementation will work, and centralizing the mapping can be addressed in the future if needed.
| filePath := filepath.Join(payloadFeatureGatePath, currFeatureSetManifestFile.Name()) | ||
| versionedFeatureSet, err := parseFeatureGateFile(filePath) | ||
| if err != nil { | ||
| // Skip files that don't contain version annotations or can't be parsed | ||
| // This allows for backward compatibility with other files in the directory | ||
| continue | ||
| } |
There was a problem hiding this comment.
Don’t silently skip parse failures for versioned feature gates.
AllVersionedFeatureSets currently ignores all errors, which can mask malformed YAML or unreadable files and silently drop versioned gates. It’s safer to only skip known “not-versioned” cases and surface everything else.
🛠️ Suggested error handling split
+var (
+ errNoVersionAnnotation = errors.New("no version annotation")
+ errNoClusterProfileAnnotation = errors.New("no cluster profile annotation")
+)
+
func parseFeatureGateFile(filePath string) (*VersionedFeatureSet, error) {
@@
- if !found {
- return nil, fmt.Errorf("no version annotation found in %s", filename)
- }
+ if !found {
+ return nil, errNoVersionAnnotation
+ }
@@
- if clusterProfile == "" {
- return nil, fmt.Errorf("no cluster profile annotation found in %s", filename)
- }
+ if clusterProfile == "" {
+ return nil, errNoClusterProfileAnnotation
+ }
@@
versionedFeatureSet, err := parseFeatureGateFile(filePath)
if err != nil {
- // Skip files that don't contain version annotations or can't be parsed
- // This allows for backward compatibility with other files in the directory
- continue
+ if errors.Is(err, errNoVersionAnnotation) || errors.Is(err, errNoClusterProfileAnnotation) {
+ continue
+ }
+ return nil, err
}🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 246 - 252, The loop
in AllVersionedFeatureSets currently swallows every error from
parseFeatureGateFile when iterating files (filePath built from
payloadFeatureGatePath and currFeatureSetManifestFile.Name()), which can hide
real parse/read failures; change the error handling so you detect and skip only
the "not-versioned" sentinel (or a dedicated ErrNotVersioned) returned by
parseFeatureGateFile, but for any other error return it (or log/propagate it)
instead of continuing, ensuring malformed YAML or IO errors surface to the
caller.
| func (f *VersionFilter) String() string { | ||
| return fmt.Sprintf("version=%d", f.targetVersion) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the VersionFilter struct definition
rg -A 10 "type VersionFilter struct" tools/codegen/pkg/manifestmerge/filters.goRepository: openshift/api
Length of output: 411
🏁 Script executed:
# Also check the exact lines 596-597 to see current state
sed -n '590,600p' tools/codegen/pkg/manifestmerge/filters.goRepository: openshift/api
Length of output: 303
Use %v with UnsortedList() when formatting a set.
The targetVersion field is of type sets.Set[uint64], not a scalar integer. Using %d will produce an incorrect representation of the set. Use %v with UnsortedList() to display the set's contents clearly.
func (f *VersionFilter) String() string {
return fmt.Sprintf("version=%v", f.targetVersion.UnsortedList())
}🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 596 - 597, The
VersionFilter.String method currently formats f.targetVersion as an integer;
update the formatting to print the set's contents by using
f.targetVersion.UnsortedList() with a %v verb so the VersionFilter.String
(function VersionFilter.String) produces a correct, human-readable
representation of the sets.Set[uint64] stored in targetVersion.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.golangci.yaml:
- Around line 109-111: The path regexp "features|payload-command/*.go" in
.golangci.yaml is malformed for golangci-lint (Go regexp), so replace it with a
proper Go regexp that groups the directories, uses ".*" for any characters and
escapes the dot; e.g., set the path to ^(features|payload-command)/.*\.go$ to
match .go files under those dirs, or use ^(features|payload-command)/ to exclude
the entire directories regardless of extension.
In `@payload-manifests/featuregates/featureGate-5-10-Hypershift-Default.yaml`:
- Around line 104-106: The feature gate name "DyanmicServiceEndpointIBMCloud" is
misspelled and must be renamed to "DynamicServiceEndpointIBMCloud" everywhere;
search for the symbol "DyanmicServiceEndpointIBMCloud" across feature gate
YAMLs, generated code, types, constants, and documentation and replace each
occurrence with "DynamicServiceEndpointIBMCloud", then update any code
references, type names, serialization keys, codegen inputs, and tests to use the
corrected name and re-run the code generation/build steps so generated artifacts
and test expectations reflect the change.
In `@tests/crd_filter.go`:
- Around line 143-159: The function versionsFrom builds versionString from
annotations and splits it, but it doesn't trim whitespace so values like "4, 5"
will cause strconv.ParseUint to fail and silently drop valid entries; update
versionsFrom to trim spaces (e.g., using strings.TrimSpace) on each split token
before skipping empty tokens and passing to strconv.ParseUint, ensuring you
still handle parse errors as before and insert the parsed uint64 into versions.
| func versionsFrom(annotations map[string]string) sets.Set[uint64] { | ||
| var versionString string | ||
| for k, v := range annotations { | ||
| if strings.HasPrefix(k, "release.openshift.io/major-version") { | ||
| versionString = v | ||
| break | ||
| } | ||
| } | ||
|
|
||
| versions := sets.New[uint64]() | ||
| for _, version := range strings.Split(versionString, ",") { | ||
| versionInt, err := strconv.ParseUint(version, 10, 64) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| versions.Insert(versionInt) | ||
| } |
There was a problem hiding this comment.
Trim whitespace before parsing versions to avoid silent drops.
If annotations ever include spaces (e.g., "4, 5"), ParseUint will fail and skip valid versions.
✅ Suggested fix
- for _, version := range strings.Split(versionString, ",") {
- versionInt, err := strconv.ParseUint(version, 10, 64)
+ for _, version := range strings.Split(versionString, ",") {
+ version = strings.TrimSpace(version)
+ if version == "" {
+ continue
+ }
+ versionInt, err := strconv.ParseUint(version, 10, 64)
if err != nil {
continue
}
versions.Insert(versionInt)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func versionsFrom(annotations map[string]string) sets.Set[uint64] { | |
| var versionString string | |
| for k, v := range annotations { | |
| if strings.HasPrefix(k, "release.openshift.io/major-version") { | |
| versionString = v | |
| break | |
| } | |
| } | |
| versions := sets.New[uint64]() | |
| for _, version := range strings.Split(versionString, ",") { | |
| versionInt, err := strconv.ParseUint(version, 10, 64) | |
| if err != nil { | |
| continue | |
| } | |
| versions.Insert(versionInt) | |
| } | |
| func versionsFrom(annotations map[string]string) sets.Set[uint64] { | |
| var versionString string | |
| for k, v := range annotations { | |
| if strings.HasPrefix(k, "release.openshift.io/major-version") { | |
| versionString = v | |
| break | |
| } | |
| } | |
| versions := sets.New[uint64]() | |
| for _, version := range strings.Split(versionString, ",") { | |
| version = strings.TrimSpace(version) | |
| if version == "" { | |
| continue | |
| } | |
| versionInt, err := strconv.ParseUint(version, 10, 64) | |
| if err != nil { | |
| continue | |
| } | |
| versions.Insert(versionInt) | |
| } |
🤖 Prompt for AI Agents
In `@tests/crd_filter.go` around lines 143 - 159, The function versionsFrom builds
versionString from annotations and splits it, but it doesn't trim whitespace so
values like "4, 5" will cause strconv.ParseUint to fail and silently drop valid
entries; update versionsFrom to trim spaces (e.g., using strings.TrimSpace) on
each split token before skipping empty tokens and passing to strconv.ParseUint,
ensuring you still handle parse errors as before and insert the parsed uint64
into versions.
features/util.go
Outdated
|
|
||
| status []featureGateStatus | ||
|
|
||
| statusByClusterProfileByFeatureSet map[ClusterProfileName]map[configv1.FeatureSet]bool |
There was a problem hiding this comment.
Looks like this still needs cleanup?
...rated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-Default.crd.yaml
Show resolved
Hide resolved
|
Overall, I think this looks pretty good. |
JoelSpeed
force-pushed
the
major-version-feature-gating
branch
from
933f310 to
095155b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/codegen/pkg/manifestmerge/filters.go (1)
375-382:⚠️ Potential issue | 🟡 MinorSame bounds check issue in
FilterForFeatureSet.Line 379 also accesses
uncastFeatureGateSlice[0]without verifying the slice is non-empty.🛡️ Proposed fix
uncastFeatureGateSlice, _, err := unstructured.NestedSlice(uncastFeatureGate, "status", "featureGates") if err != nil { return nil, fmt.Errorf("no slice found %w", err) } + if len(uncastFeatureGateSlice) == 0 { + return nil, fmt.Errorf("empty featureGates slice in %s", featureGateFilename) + } enabledFeatureGates, _, err := unstructured.NestedSlice(uncastFeatureGateSlice[0].(map[string]interface{}), "enabled")tests/crd_filter.go (1)
221-232:⚠️ Potential issue | 🟡 MinorPotential panic: accessing slice index without bounds check.
Lines 225 and 229 access
uncastFeatureGateSlice[0]without verifying the slice is non-empty. If a feature gate YAML has an emptystatus.featureGatesarray, this will panic.🛡️ Proposed fix
uncastFeatureGateSlice, _, err := unstructured.NestedSlice(uncastFeatureGate, "status", "featureGates") if err != nil { return nil, fmt.Errorf("no slice found %w", err) } + if len(uncastFeatureGateSlice) == 0 { + return nil, fmt.Errorf("empty featureGates slice in matched feature gate for cluster profile %q, feature set %q", clusterProfile, featureSetName) + } enabledFeatureGates, _, err := unstructured.NestedSlice(uncastFeatureGateSlice[0].(map[string]interface{}), "enabled")
🤖 Fix all issues with AI agents
In `@tools/codegen/pkg/manifestmerge/filters.go`:
- Around line 454-461: The code indexes uncastFeatureGateSlice[0] without
verifying the slice has any elements which can panic; update the logic around
the unstructured.NestedSlice call that produces uncastFeatureGateSlice to check
its length (e.g., if len(uncastFeatureGateSlice) == 0) and return a descriptive
error before attempting to call unstructured.NestedSlice on
uncastFeatureGateSlice[0]. Ensure the error message references the missing
status.featureGates entry and keep the subsequent enabledFeatureGates lookup
unchanged once the bounds check passes.
🧹 Nitpick comments (1)
features/features.go (1)
43-71: Consider deterministic iteration order forAllFeatureSets.The function uses
for version := range versions(line 60) and similar range loops over sets. Sincesets.Setiteration order is non-deterministic, the returned map will have entries populated in arbitrary order. While this doesn't affect correctness (maps don't preserve order), it could cause non-deterministic behavior in downstream code that iterates the result if that code expects consistent ordering.If deterministic output is important for manifest generation stability, consider sorting the versions before iteration.
♻️ Optional: Sort versions for deterministic iteration
- for version := range versions { + sortedVersions := versions.UnsortedList() + sort.Slice(sortedVersions, func(i, j int) bool { return sortedVersions[i] < sortedVersions[j] }) + for _, version := range sortedVersions {
| uncastFeatureGateSlice, _, err := unstructured.NestedSlice(uncastFeatureGate, "status", "featureGates") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("no slice found %w", err) | ||
| } | ||
| enabledFeatureGates, _, err := unstructured.NestedSlice(uncastFeatureGateSlice[0].(map[string]interface{}), "enabled") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("no enabled found %w", err) | ||
| } |
There was a problem hiding this comment.
Potential panic: accessing slice index without bounds check.
Line 458 accesses uncastFeatureGateSlice[0] without verifying the slice is non-empty. If the YAML has an empty status.featureGates array, this will panic.
🛡️ Proposed fix to add bounds check
uncastFeatureGateSlice, _, err := unstructured.NestedSlice(uncastFeatureGate, "status", "featureGates")
if err != nil {
return nil, fmt.Errorf("no slice found %w", err)
}
+ if len(uncastFeatureGateSlice) == 0 {
+ return nil, fmt.Errorf("empty featureGates slice in %s", featureGateFilename)
+ }
enabledFeatureGates, _, err := unstructured.NestedSlice(uncastFeatureGateSlice[0].(map[string]interface{}), "enabled")🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/filters.go` around lines 454 - 461, The code
indexes uncastFeatureGateSlice[0] without verifying the slice has any elements
which can panic; update the logic around the unstructured.NestedSlice call that
produces uncastFeatureGateSlice to check its length (e.g., if
len(uncastFeatureGateSlice) == 0) and return a descriptive error before
attempting to call unstructured.NestedSlice on uncastFeatureGateSlice[0]. Ensure
the error message references the missing status.featureGates entry and keep the
subsequent enabledFeatureGates lookup unchanged once the bounds check passes.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A handful of the coderabbit comments seem like reasonable things to address, but I also don't think they are necessarily blocking in my eyes.
I think that the current state shows that it is working as intended, and if we encounter edge cases and bugs we can follow up in the future.
I'll tag, but add a hold in case you want to comb through the coderabbit suggestions and take any action on them.
/lgtm
/hold
openshift-ci
bot
added
the
do-not-merge/hold
openshift-ci
bot
added
lgtm
JoelSpeed
force-pushed
the
major-version-feature-gating
branch
2 times, most recently
from
11d61ed to
c1727b7
Compare
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A handful of comments, but nothing that I think is worth blocking this on at the moment.
The only one I'd be most keen to look into would be the strings.HasPrefix usage instead of equivalence check for the release.openshift.io/major-version annotation, but it is unlikely we hit that issue any time soon so not blocking in favor of getting this in.
/lgtm
| for _, featureSet := range strings.Split(featureSetAnnotation, ",") { | ||
| featureSets.Insert(featureSet) | ||
| } |
There was a problem hiding this comment.
Non-blocking nit: this explicit loop isn't necessary. Could do something like:
| for _, featureSet := range strings.Split(featureSetAnnotation, ",") { | |
| featureSets.Insert(featureSet) | |
| } | |
| featureSets.Insert(strings.Split(featureSetAnnotation, ",")...) |
| func versionsFrom(annotations map[string]string) sets.Set[uint64] { | ||
| var versionString string | ||
| for k, v := range annotations { | ||
| if strings.HasPrefix(k, "release.openshift.io/major-version") { |
There was a problem hiding this comment.
Any particular reason we want to use strings.HasPrefix here instead of an explicit ==? If we ever add a new annotation like release.openshift.io/major-version-X (where X is anything else), this would match that which seems like something we wouldn't want?
The likelihood of us doing that is probably low, but this logic could need to evolve over time and using an explicit equivalence check seems less brittle in this case.
| for _, v := range vr { | ||
| if v == version { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
This works fine as-is, so not blocking but https://pkg.go.dev/slices#Contains is something that would probably work here as a straightforward:
return slices.Contains(vr, version)Not entirely sure, but might be able to replace this entire method with using that instead.
| Object: featureGateMap, | ||
| } | ||
|
|
||
| // Extract cluster profile from annotations |
There was a problem hiding this comment.
Seems like a stale comment?
| // Determine cluster profile from the annotations | ||
| var clusterProfile string | ||
| for annotationKey, annotationValue := range annotations { | ||
| if annotationValue == "false-except-for-the-config-operator" { | ||
| // Convert long cluster profile names to short names | ||
| switch annotationKey { | ||
| case "include.release.openshift.io/ibm-cloud-managed": | ||
| clusterProfile = "Hypershift" | ||
| case "include.release.openshift.io/self-managed-high-availability": | ||
| clusterProfile = "SelfManagedHA" | ||
| default: | ||
| clusterProfile = annotationKey // fallback to the key itself | ||
| } |
There was a problem hiding this comment.
I don't disagree that a centralized definition for the translations here would be nice, but I wouldn't block on this.
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
|
Scheduling tests matching the |
openshift-merge-robot
added
the
needs-rebase
JoelSpeed
force-pushed
the
major-version-feature-gating
branch
from
885ba4c to
00bee77
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/verify-crd-schema |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema DetailsIn response to this:
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. |
|
/retest |
1 similar comment
|
/retest |
|
@JoelSpeed: 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. |
4 participants
This PR works in tandem with openshift/cluster-version-operator#1282 and an as yet unbuilt config operator PR to allow us to generate different versions of feature gates based on the target major version of the CVO payload.
This will allow us to have manifests that we deploy only in 4, or only in 5, or in a combination of releases. Importantly, it means that we can start setting up multiple major versions in development from the same branches, and gate CVO manifests, and feature gates, based on which major version is built into the build stream.