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 10 commits intoopenshift:masterfrom
Conversation
📝 WalkthroughWalkthroughThe patch makes feature-gating version-aware across the codebase. FeatureSets now accepts a major-version and returns per-version Enabled/Disabled results; AllFeatureSets is keyed by version. Internal gate registration moved to a version/cluster-profile/feature-set DSL and allFeatureGates now stores per-gate status slices. Payload rendering parses semver, passes payload major version to gate resolution, and write_featureset consolidates consecutive versions into grouped manifests. tools/codegen gained VersionedFeatureSet/VersionRange and version-aware filters/generation. Many FeatureGate YAMLs and CRDs were added, removed, or had annotations updated; tests and scripts were adjusted accordingly. 🚥 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.
|
|
||
| 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
force-pushed
the
major-version-feature-gating
branch
from
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).
| // featureGateContentEqual compares two FeatureGateEnabledDisabled structs for content equality | ||
| func featureGateContentEqual(a, b *features.FeatureGateEnabledDisabled) bool { | ||
| if a == nil && b == nil { | ||
| return true | ||
| } | ||
| if a == nil || b == nil { | ||
| return false | ||
| } | ||
|
|
||
| // Create sorted slices of feature gate names for comparison | ||
| aEnabledNames := sets.New(a.Enabled...) | ||
| bEnabledNames := sets.New(b.Enabled...) | ||
|
|
||
| if !aEnabledNames.Equal(bEnabledNames) { | ||
| return false | ||
| } | ||
|
|
||
| aDisabledNames := sets.New(a.Disabled...) | ||
| bDisabledNames := sets.New(b.Disabled...) | ||
| if !aDisabledNames.Equal(bDisabledNames) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the FeatureGateDescription struct definition
ast-grep --pattern 'type FeatureGateDescription struct {
$$$
}'Repository: openshift/api
Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Find the FeatureGateEnabledDisabled struct definition
ast-grep --pattern 'type FeatureGateEnabledDisabled struct {
$$$
}'Repository: openshift/api
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Find FeatureGateAttributes struct definition (from configv1 package)
rg 'type FeatureGateAttributes struct' -A 20Repository: openshift/api
Length of output: 1392
🏁 Script executed:
#!/bin/bash
# Find OwningProduct type definition
rg 'type OwningProduct' -A 10Repository: openshift/api
Length of output: 444
The comparison logic may fail if FeatureGateDescription instances differ only in metadata fields.
sets.New(a.Enabled...) compares entire FeatureGateDescription structs for equality. Since FeatureGateDescription contains metadata fields (EnhancementPR, OwningJiraComponent, ResponsiblePerson, OwningProduct) beyond the gate's Name, two instances with identical names but different metadata would be treated as unequal. This causes the comparison to return false even when the meaningful content (which gates are enabled/disabled) is identical.
Consider comparing only the gate names instead, or ensure that these metadata fields are identical across instances when they should represent the same gate.
🤖 Prompt for AI Agents
In `@payload-command/render/write_featureset.go` around lines 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).
| // 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.
| 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.
| case allFeatureSetsSame: | ||
| for _, clusterProfile := range crd.clusterProfile.UnsortedList() { | ||
| clusterProfileShortName, err := utils.ClusterProfileToShortName(clusterProfile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unrecognized clusterprofile name %q: %w", clusterProfile, err) | ||
| } | ||
| nameComponents = append(nameComponents, clusterProfileShortName) | ||
| crdFilename := getCRDFilename(crdFilenamePattern, nameComponents) | ||
| crdFullPath := filepath.Join(outputPath, crdFilename) | ||
|
|
||
| crdsToWrite = append(crdsToWrite, crdForFeatureSet{ | ||
| crd: crd.crd.DeepCopy(), | ||
| // featureSet: Don't write the featureset as it's the same for all CRDs. | ||
| clusterProfile: sets.New[string](clusterProfile), | ||
| version: crd.version, | ||
| outputFile: crdFullPath, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Bug: nameComponents slice mutation corrupts subsequent iterations.
Line 532 appends to nameComponents in the loop over crd.clusterProfile.UnsortedList(). Since Go slices share underlying arrays until reallocation, this mutation persists across iterations, causing incorrect filename generation for subsequent cluster profiles.
🐛 Proposed fix
case allFeatureSetsSame:
for _, clusterProfile := range crd.clusterProfile.UnsortedList() {
clusterProfileShortName, err := utils.ClusterProfileToShortName(clusterProfile)
if err != nil {
return nil, fmt.Errorf("unrecognized clusterprofile name %q: %w", clusterProfile, err)
}
- nameComponents = append(nameComponents, clusterProfileShortName)
- crdFilename := getCRDFilename(crdFilenamePattern, nameComponents)
+ profileComponents := append(slices.Clone(nameComponents), clusterProfileShortName)
+ crdFilename := getCRDFilename(crdFilenamePattern, profileComponents)📝 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.
| case allFeatureSetsSame: | |
| for _, clusterProfile := range crd.clusterProfile.UnsortedList() { | |
| clusterProfileShortName, err := utils.ClusterProfileToShortName(clusterProfile) | |
| if err != nil { | |
| return nil, fmt.Errorf("unrecognized clusterprofile name %q: %w", clusterProfile, err) | |
| } | |
| nameComponents = append(nameComponents, clusterProfileShortName) | |
| crdFilename := getCRDFilename(crdFilenamePattern, nameComponents) | |
| crdFullPath := filepath.Join(outputPath, crdFilename) | |
| crdsToWrite = append(crdsToWrite, crdForFeatureSet{ | |
| crd: crd.crd.DeepCopy(), | |
| // featureSet: Don't write the featureset as it's the same for all CRDs. | |
| clusterProfile: sets.New[string](clusterProfile), | |
| version: crd.version, | |
| outputFile: crdFullPath, | |
| }) | |
| } | |
| case allFeatureSetsSame: | |
| for _, clusterProfile := range crd.clusterProfile.UnsortedList() { | |
| clusterProfileShortName, err := utils.ClusterProfileToShortName(clusterProfile) | |
| if err != nil { | |
| return nil, fmt.Errorf("unrecognized clusterprofile name %q: %w", clusterProfile, err) | |
| } | |
| profileComponents := append(slices.Clone(nameComponents), clusterProfileShortName) | |
| crdFilename := getCRDFilename(crdFilenamePattern, profileComponents) | |
| crdFullPath := filepath.Join(outputPath, crdFilename) | |
| crdsToWrite = append(crdsToWrite, crdForFeatureSet{ | |
| crd: crd.crd.DeepCopy(), | |
| // featureSet: Don't write the featureset as it's the same for all CRDs. | |
| clusterProfile: sets.New[string](clusterProfile), | |
| version: crd.version, | |
| outputFile: crdFullPath, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@tools/codegen/pkg/manifestmerge/generator.go` around lines 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).
|
@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. |
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.
| - linters: | ||
| - kubeapilinter | ||
| path: features|payload-command/*.go |
There was a problem hiding this comment.
Regex pattern appears malformed.
The path pattern features|payload-command/*.go uses glob-style * syntax, but golangci-lint uses Go regexp where * means "zero or more of the preceding character" (not "any characters"). Additionally, the . before go matches any single character rather than a literal dot.
If the intent is to exclude all .go files under features/ and payload-command/ directories, consider:
🔧 Proposed fix
- linters:
- kubeapilinter
- path: features|payload-command/*.go
+ path: (features|payload-command)/.*\.goAlternatively, if you want to exclude entire directories regardless of file extension:
path: (features|payload-command)/📝 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.
| - linters: | |
| - kubeapilinter | |
| path: features|payload-command/*.go | |
| - linters: | |
| - kubeapilinter | |
| path: (features|payload-command)/.*\.go |
🤖 Prompt for AI Agents
In @.golangci.yaml around lines 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.
| { | ||
| "name": "AzureDedicatedHosts" | ||
| "name": "DyanmicServiceEndpointIBMCloud" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the typo "Dyanmic" exists elsewhere or if "Dynamic" is the correct spelling
echo "=== Searching for 'DyanmicServiceEndpointIBMCloud' (potential typo) ==="
rg -n "DyanmicServiceEndpointIBMCloud"
echo ""
echo "=== Searching for 'DynamicServiceEndpointIBMCloud' (expected correct spelling) ==="
rg -n "DynamicServiceEndpointIBMCloud"Repository: openshift/api
Length of output: 3985
Fix typo: "DyanmicServiceEndpointIBMCloud" should be "DynamicServiceEndpointIBMCloud".
This misspelling appears consistently across the codebase in multiple files (feature gate definitions, code generation, types, and documentation). Update all instances to use the correct spelling "Dynamic" instead of "Dyanmic".
🤖 Prompt for AI Agents
In `@payload-manifests/featuregates/featureGate-5-10-Hypershift-Default.yaml`
around lines 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.
| 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.
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.