NO-JIRA: Add feature gate changes to changelog JSON output#2247
NO-JIRA: Add feature gate changes to changelog JSON output#2247stbenjam wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughdescribeChangelog now computes feature-set diffs for JSON output, reports diff errors to errOut and sets hasError, and populates ChangeLog.FeatureGates with per-feature-gate status keyed by cluster-profile and feature-set; a new ChangeLogFeatureGateInfo type and FeatureGates field were added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stbenjam 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/release/info.go`:
- Around line 1780-1802: The code silently ignores errors from
calculateFeatureSetDiff (the if uses err == nil) which can drop FeatureGates
without warning; change the block around calculateFeatureSetDiff so failures are
surfaced instead of ignored — e.g., if calculateFeatureSetDiff returns an error,
propagate or return that error (or log it and attach the error to the response)
rather than skipping population of changeLog.FeatureGates; adjust the
surrounding function (the caller that currently uses
featureSetDiff/GetOrderedFeatureGates/AllFeatureSets/AllClusterProfiles/FeatureInfoFor
and appends to changeLog.FeatureGates) to handle and return the error so callers
receive a clear failure instead of incomplete JSON.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4a00f493-a878-419b-be9d-d5a2720ca23a
📒 Files selected for processing (1)
pkg/cli/admin/release/info.go
stbenjam
force-pushed
the
fg-diff
branch
2 times, most recently
from
b0bd727 to
edce8fb
Compare
|
It would be lovely to also add some tests, please. |
stbenjam
force-pushed
the
fg-diff
branch
2 times, most recently
from
17a150b to
17e54ca
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cli/admin/release/info_changelog_test.go (1)
45-186: Please cover the new failure path too.These tests hit the success and empty-input cases, but not the new error branch at Line 1780. One malformed-manifest case should assert that
describeChangelog(..., "json")returnsErrExitand reports the feature-gate diff failure on stderr, so this behavior doesn't regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/release/info_changelog_test.go` around lines 45 - 186, Add a new unit test (e.g., TestDescribeChangelogFeatureGatesJSONMalformed) that exercises the error branch in describeChangelog when parsing a malformed manifest: build a ReleaseDiff where To or From.ManifestFiles contains an invalid feature-gate YAML byte slice, call describeChangelog(out, errOut, &ReleaseInfo{}, diff, "", "json"), assert the returned error equals ErrExit, and assert errOut.String() contains a short identifying substring like "feature-gate" or "feature gate" (to verify the feature-gate diff failure was reported). Use the same helpers/structures from the file (featureGateManifest pattern, ReleaseDiff, ReleaseInfo, ChangeLog) to locate relevant code paths and keep the test style consistent with TestDescribeChangelogFeatureGatesJSON and TestDescribeChangelogNoFeatureGatesJSON.pkg/cli/admin/release/info.go (1)
1786-1803: Prune empty profile buckets and gates before appending toFeatureGates.
AllClusterProfiles()aggregates the union of all cluster profiles in the diff, so the loop at line 1794 eagerly allocates a status map for every profile. If a gate has no actual changes for a cluster profile (or only changes in the filteredLatencySensitivefeature set), it creates empty{}entries. Worse, if a gate ends up with no statuses at all after filtering, it still gets appended, making the JSON unnecessarily noisy for downstream consumers.Collect statuses conditionally—only add a profile's bucket if it contains changes, and only append the gate entry if it has at least one profile with changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/admin/release/info.go` around lines 1786 - 1803, The loop building ChangeLogFeatureGateInfo for each gate (involving orderedFeatureGates, allClusterProfiles, allFeatureSets and featureSetDiff.FeatureInfoFor) eagerly creates empty profile buckets and appends gates with no actual statuses to changeLog.FeatureGates; fix by only creating fgInfo.Status[cp] when you find at least one change for that profile (i.e., add cp only when diffInfo.ChangedFeatureGates[fg] exists and produces a non-empty value), and after scanning all profiles, append fgInfo to changeLog.FeatureGates only if fgInfo.Status is non-empty (has >=1 profile key). Ensure you still skip the "LatencySensitive" featureset as before when evaluating allFeatureSets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cli/admin/release/info_changelog_test.go`:
- Around line 45-186: Add a new unit test (e.g.,
TestDescribeChangelogFeatureGatesJSONMalformed) that exercises the error branch
in describeChangelog when parsing a malformed manifest: build a ReleaseDiff
where To or From.ManifestFiles contains an invalid feature-gate YAML byte slice,
call describeChangelog(out, errOut, &ReleaseInfo{}, diff, "", "json"), assert
the returned error equals ErrExit, and assert errOut.String() contains a short
identifying substring like "feature-gate" or "feature gate" (to verify the
feature-gate diff failure was reported). Use the same helpers/structures from
the file (featureGateManifest pattern, ReleaseDiff, ReleaseInfo, ChangeLog) to
locate relevant code paths and keep the test style consistent with
TestDescribeChangelogFeatureGatesJSON and
TestDescribeChangelogNoFeatureGatesJSON.
In `@pkg/cli/admin/release/info.go`:
- Around line 1786-1803: The loop building ChangeLogFeatureGateInfo for each
gate (involving orderedFeatureGates, allClusterProfiles, allFeatureSets and
featureSetDiff.FeatureInfoFor) eagerly creates empty profile buckets and appends
gates with no actual statuses to changeLog.FeatureGates; fix by only creating
fgInfo.Status[cp] when you find at least one change for that profile (i.e., add
cp only when diffInfo.ChangedFeatureGates[fg] exists and produces a non-empty
value), and after scanning all profiles, append fgInfo to changeLog.FeatureGates
only if fgInfo.Status is non-empty (has >=1 profile key). Ensure you still skip
the "LatencySensitive" featureset as before when evaluating allFeatureSets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d3f3d351-4294-47af-a5c4-76fc87557555
📒 Files selected for processing (2)
pkg/cli/admin/release/info.gopkg/cli/admin/release/info_changelog_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/admin/release/info.go`:
- Around line 1784-1803: The code currently preallocates cluster-profile buckets
and may append empty ChangeLogFeatureGateInfo entries; modify the loop over
orderedFeatureGates so you first skip or remove the "LatencySensitive" gate (or
filter orderedFeatureGates) and then only create fgInfo.Status[cp] inside the
inner loop when you actually observe a change (i.e. when
featureSetDiff.FeatureInfoFor(cp, fs) yields a diff and
diffInfo.ChangedFeatureGates[fg] exists); after processing all profiles, only
append fgInfo to changeLog.FeatureGates if fgInfo.Status is non-empty (has at
least one profile key). This uses orderedFeatureGates, allFeatureSets (after
Delete), allClusterProfiles, FeatureInfoFor and ChangeLogFeatureGateInfo to
locate and fix the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ff300291-ea3f-4e6f-920c-02d0562c6069
📒 Files selected for processing (2)
pkg/cli/admin/release/info.gopkg/cli/admin/release/info_changelog_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/admin/release/info_changelog_test.go
The changelog JSON output (`oc adm release info --changelog --output=json`) was missing feature gate change data that was only available in the markdown output. This adds a `featureGates` field to the ChangeLog JSON struct, populated using the existing `calculateFeatureSetDiff()` logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@tchap I added some tests and pulled out the shared Latency feature set into a helper, with a comment explaining it. |
|
/retitle NO-JIRA: Add feature gate changes to changelog JSON output |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@stbenjam: This pull request explicitly references no jira issue. 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. |
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
@stbenjam: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
- Use json.NewDecoder instead of json.Unmarshal - Remove unused featureSet parameter from featureGateManifest - Use cmp.Diff with expected objects instead of manual assertions
|
Updated. |
|
/verified by stbenjam |
openshift-ci-robot
added
the
verified
|
@stbenjam: This PR has been marked as verified by 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. |
3 participants
Summary
oc adm release info --changelog --output=json) was missing feature gate change data that was only available in the markdown outputfeatureGatesfield to theChangeLogJSON struct using the existingcalculateFeatureSetDiff()logicSample JSON output
{ "featureGates": [ { "name": "GatewayAPI", "status": { "Hypershift": { "Default": "Unconditionally Enabled (Changed)", "DevPreviewNoUpgrade": "Unconditionally Enabled (Changed)", "TechPreviewNoUpgrade": "Unconditionally Enabled (Changed)" }, "SelfManagedHA": { "Default": "Unconditionally Enabled (Changed)", "DevPreviewNoUpgrade": "Unconditionally Enabled (Changed)", "TechPreviewNoUpgrade": "Unconditionally Enabled (Changed)" } } }, { "name": "MachineConfigNodes", "status": { "Hypershift": { "Default": "Unconditionally Enabled (Changed)" }, "SelfManagedHA": { "Default": "Unconditionally Enabled (Changed)" } } } ] }🤖 Generated with Claude Code