OCPSTRAT-2876: Enable major version segmentation of enabled feature gates #2637
Conversation
📝 WalkthroughWalkthroughThis PR makes feature-gate evaluation version-aware and reorganizes data structures and generation flows. FeatureSets now accepts a payload major-version and returns aggregated enabled/disabled gates; AllFeatureSets is version-keyed. allFeatureGates storage moves to map[FeatureGateName][]featureGateStatus and feature registration uses option-based enable(...) predicates. Payload rendering and writers propagate the major version, consolidate consecutive identical versions into ranges, and add a release.openshift.io/major-version annotation to generated FeatureGate manifests. tools/codegen is extended for versioned feature-set discovery and per-version CRD/manifest generation. A semver dependency and a cleanup step in the payload writer script were added. 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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.
| // consolidateVersions groups consecutive versions with identical content | ||
| func consolidateVersions( | ||
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | ||
| clusterProfile features.ClusterProfileName, | ||
| featureSetName configv1.FeatureSet, | ||
| ) []versionGroup { | ||
| var versionContents []versionGroup | ||
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | ||
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | ||
| if content, exists := byFeatureSet[featureSetName]; exists { | ||
| versionContents = append(versionContents, versionGroup{ | ||
| startVersion: version, | ||
| content: content, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Sort by version | ||
| sort.Slice(versionContents, func(i, j int) bool { | ||
| return versionContents[i].startVersion < versionContents[j].startVersion | ||
| }) | ||
|
|
||
| var groups []versionGroup | ||
|
|
||
| currentGroup := versionContents[0] | ||
| currentGroup.endVersion = currentGroup.startVersion | ||
|
|
||
| for i := 1; i < len(versionContents); i++ { | ||
| curr := versionContents[i] | ||
|
|
||
| // Check if current version has same content as current group and is consecutive | ||
| if featureGateContentEqual(currentGroup.content, curr.content) && | ||
| curr.startVersion == currentGroup.endVersion+1 { | ||
| // Extend current group | ||
| currentGroup.endVersion = curr.startVersion | ||
| } else { | ||
| // Finalize current group and start new one | ||
| groups = append(groups, currentGroup) | ||
| currentGroup = curr | ||
| currentGroup.endVersion = currentGroup.startVersion | ||
| } | ||
| } | ||
|
|
||
| // Add the final group | ||
| groups = append(groups, currentGroup) | ||
|
|
||
| return groups | ||
| } |
There was a problem hiding this comment.
Panic on empty versionContents slice.
If no matching entries exist for the given clusterProfile and featureSetName, versionContents will be empty, causing a panic at line 99 when accessing versionContents[0].
🔎 Proposed fix
// Sort by version
sort.Slice(versionContents, func(i, j int) bool {
return versionContents[i].startVersion < versionContents[j].startVersion
})
+ if len(versionContents) == 0 {
+ return nil
+ }
+
var groups []versionGroup
currentGroup := versionContents[0]📝 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.
| // consolidateVersions groups consecutive versions with identical content | |
| func consolidateVersions( | |
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | |
| clusterProfile features.ClusterProfileName, | |
| featureSetName configv1.FeatureSet, | |
| ) []versionGroup { | |
| var versionContents []versionGroup | |
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | |
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | |
| if content, exists := byFeatureSet[featureSetName]; exists { | |
| versionContents = append(versionContents, versionGroup{ | |
| startVersion: version, | |
| content: content, | |
| }) | |
| } | |
| } | |
| } | |
| // Sort by version | |
| sort.Slice(versionContents, func(i, j int) bool { | |
| return versionContents[i].startVersion < versionContents[j].startVersion | |
| }) | |
| var groups []versionGroup | |
| currentGroup := versionContents[0] | |
| currentGroup.endVersion = currentGroup.startVersion | |
| for i := 1; i < len(versionContents); i++ { | |
| curr := versionContents[i] | |
| // Check if current version has same content as current group and is consecutive | |
| if featureGateContentEqual(currentGroup.content, curr.content) && | |
| curr.startVersion == currentGroup.endVersion+1 { | |
| // Extend current group | |
| currentGroup.endVersion = curr.startVersion | |
| } else { | |
| // Finalize current group and start new one | |
| groups = append(groups, currentGroup) | |
| currentGroup = curr | |
| currentGroup.endVersion = currentGroup.startVersion | |
| } | |
| } | |
| // Add the final group | |
| groups = append(groups, currentGroup) | |
| return groups | |
| } | |
| // consolidateVersions groups consecutive versions with identical content | |
| func consolidateVersions( | |
| statusByVersionByClusterProfileByFeatureSet map[uint64]map[features.ClusterProfileName]map[configv1.FeatureSet]*features.FeatureGateEnabledDisabled, | |
| clusterProfile features.ClusterProfileName, | |
| featureSetName configv1.FeatureSet, | |
| ) []versionGroup { | |
| var versionContents []versionGroup | |
| for version, byClusterProfile := range statusByVersionByClusterProfileByFeatureSet { | |
| if byFeatureSet, exists := byClusterProfile[clusterProfile]; exists { | |
| if content, exists := byFeatureSet[featureSetName]; exists { | |
| versionContents = append(versionContents, versionGroup{ | |
| startVersion: version, | |
| content: content, | |
| }) | |
| } | |
| } | |
| } | |
| // Sort by version | |
| sort.Slice(versionContents, func(i, j int) bool { | |
| return versionContents[i].startVersion < versionContents[j].startVersion | |
| }) | |
| if len(versionContents) == 0 { | |
| return nil | |
| } | |
| var groups []versionGroup | |
| currentGroup := versionContents[0] | |
| currentGroup.endVersion = currentGroup.startVersion | |
| for i := 1; i < len(versionContents); i++ { | |
| curr := versionContents[i] | |
| // Check if current version has same content as current group and is consecutive | |
| if featureGateContentEqual(currentGroup.content, curr.content) && | |
| curr.startVersion == currentGroup.endVersion+1 { | |
| // Extend current group | |
| currentGroup.endVersion = curr.startVersion | |
| } else { | |
| // Finalize current group and start new one | |
| groups = append(groups, currentGroup) | |
| currentGroup = curr | |
| currentGroup.endVersion = currentGroup.startVersion | |
| } | |
| } | |
| // Add the final group | |
| groups = append(groups, currentGroup) | |
| return groups | |
| } |
🤖 Prompt for AI Agents
In payload-command/render/write_featureset.go around lines 74 to 122, the
function can panic when versionContents is empty because it unconditionally
accesses versionContents[0]; fix by checking if len(versionContents) == 0 and
return an empty groups slice immediately. Ensure the early-return occurs before
accessing versionContents[0], preserving existing behavior for non-empty slices
(sorting and grouping) unchanged.
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.
| // Generating this many versions future proofs us until at least 2040. | ||
| versions := sets.New[uint64](4, 5, 6, 7, 8, 9, 10) |
There was a problem hiding this comment.
Do we actually want per version configurations like this?
What if instead we did version ranged configurations like >=5 to represent that a feature is enabled in all OpenShift versions greater than or equal to major version 5?
I know feature gates aren't meant to stick around long term so we realistically shouldn't have feature gates that stick around a while, but something like a semver version range seems like it would future proof us indefinitely?
There was a problem hiding this comment.
I can explore this, but I expect it would be YAGNI. Feature gates that have differing behaviours between versions will not be the norm, and should always be relatively short lived. I wouldn't expect them to survive the 3 years between major version cycles
For this to work, we also need CVO to support the same semantic, so I'll pose that question on the CVO PR too.
Adding logic here does complicate things though, as we could then end up with competing clauses inadvertently. Enabling in <=4 and then >=5 would negate each others effect right?
There was a problem hiding this comment.
Enabling in <=4 and then >=5 would negate each others effect right?
Maybe? I think it would depend on how you handle version ranges and I would expect that PR reviews would be used to identify and remedy contradiction scenarios. I think that that <=4 and >=5 could be valid if they have different feature set enablements.
In my head, I'm envisioning that semver-range-enablement would be a first-satisfied approach. So the first enablement criteria that successfully matches would be used - if none, it isn't enabled at all.
|
|
||
| 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
| 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.
| for _, featureSet := range crd.featureSet.UnsortedList() { | ||
| nameComponents := append(nameComponents, clusterProfileShortName, featureSet) |
There was a problem hiding this comment.
Bug: Same nameComponents shadowing issue in default case.
Line 565 also shadows nameComponents, potentially excluding version info from the filename.
🐛 Proposed fix
for _, clusterProfile := range crd.clusterProfile.UnsortedList() {
// ...
for _, featureSet := range crd.featureSet.UnsortedList() {
- nameComponents := append(nameComponents, clusterProfileShortName, featureSet)
+ combinedComponents := append(slices.Clone(nameComponents), clusterProfileShortName, featureSet)
- crdFilename := getCRDFilename(crdFilenamePattern, nameComponents)
+ crdFilename := getCRDFilename(crdFilenamePattern, combinedComponents)🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/generator.go` around lines 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).
|
@JoelSpeed: The following tests 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.