OCPBUGS-60989: pkg/cli/admin/release/info: Mention elided first-parent, non-merge commits #2086
Conversation
openshift-ci-robot
added
jira/severity-moderate
WalkthroughAdds tracking of elided commits (from squash/rebase) to merge-log computation and propagates the count through changelog generation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI: admin release
participant Info as commitsForRepo
participant Git as mergeLogForRepo
participant Out as Output Renderer
User->>CLI: generate changelog/bugs
CLI->>Info: commitsForRepo(dir, change)
Info->>Git: mergeLogForRepo(repo, from, to)
Git-->>Info: merges[], elidedCommits, err
alt err
Info-->>CLI: error
CLI-->>User: report error
else success
Info-->>CLI: url, merges[], elidedCommits
CLI->>Out: render merges[] (+ elidedCommits if >0)
Out-->>User: changelog/bugs (includes elided commits count)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@wking: This pull request references Jira Issue OCPBUGS-60989, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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-ci
bot
requested review from
ardaguclu,
ingvagabund and
zhouying7780
openshift-ci
bot
added
the
approved
wking
force-pushed
the
adm-release-info-mention-elided-commits
branch
from
7683487 to
74dda80
Compare
![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)
pkg/cli/admin/release/git.go (1)
246-270: Count elided first-parent non-merge commits; avoid shadowing for clarityThe approach is sound. Minor readability nit: avoid shadowing args/out in this block.
Apply this refactor for clarity:
- args := []string{"log", "--no-merges", "--first-parent", "-z", "--format=%H", fmt.Sprintf("%s..%s", from, to)} - out, err := g.exec(args...) + noMergeArgs := []string{"log", "--no-merges", "--first-parent", "-z", "--format=%H", fmt.Sprintf("%s..%s", from, to)} + noMergeOut, err := g.exec(noMergeArgs...) if err != nil { return nil, 0, gitOutputToError(err, out) } - elidedCommits = strings.Count(out, "\x00") + elidedCommits = strings.Count(noMergeOut, "\x00")pkg/cli/admin/release/info.go (2)
1962-1963: Bug listing ignores elided-only commits — confirm intentIf a repo mixes merges and first-parent non-merge commits, bugs referenced only in the elided commits won’t surface in --bugs output. If that’s intentional, ignore. Otherwise, consider optionally scanning first-parent non-merge commits for bug refs when merges exist.
2486-2500: commitsForRepo: consistent triple return; minor error-message nitsBehavior is correct. Minor style nit: unify error message casing (“the source…”, “could not…”) to match surrounding errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/cli/admin/release/git.go(5 hunks)pkg/cli/admin/release/git_test.go(2 hunks)pkg/cli/admin/release/info.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cli/admin/release/git_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cli/admin/release/info.go (1)
pkg/cli/admin/release/git.go (1)
MergeCommit(189-199)
🔇 Additional comments (7)
pkg/cli/admin/release/git.go (4)
218-221: Signature update to surface elided commits: LGTMReturning (commits, elidedCommits, err) is clear, and the early no-op (from == to) path is correct.
228-243: Consistent error propagation with elided count contextGood retries and clearer errors; return shape is consistent across paths.
283-288: Preserve elidedCommits on parse errors: LGTMGood that errors keep context via elidedCommits.
330-331: Final return includes elided count: LGTMClear and matches callers.
pkg/cli/admin/release/info.go (3)
1797-1808: JSON changelog now carries elided commit count: LGTMCorrectly includes ElidedCommits and guards on (len(commits) > 0 || elided > 0).
1906-1943: Human/markdown output mentions elided commits: LGTMClear wording and placement; good UX.
2739-2741: ChangeLogImageInfo.ElidedCommits field: LGTMAppropriate JSON tag (omitempty) and type.
| for _, entry := range strings.Split(out, "\x00") { | ||
| records := strings.Split(entry, "\x1e") | ||
| if len(records) != 4 { | ||
| return nil, fmt.Errorf("unexpected git log output width %d columns", len(records)) | ||
| return nil, elidedCommits, fmt.Errorf("unexpected git log output width %d columns", len(records)) | ||
| } | ||
| unixTS, err := strconv.ParseInt(records[1], 10, 64) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden parsing against trailing NUL entries
git log -z commonly NUL-terminates entries; a trailing NUL would yield an empty entry causing the width check to fail. Skip empty entries to be robust across Git versions.
- for _, entry := range strings.Split(out, "\x00") {
+ for _, entry := range strings.Split(out, "\x00") {
+ if len(entry) == 0 {
+ continue
+ }📝 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.
| for _, entry := range strings.Split(out, "\x00") { | |
| records := strings.Split(entry, "\x1e") | |
| if len(records) != 4 { | |
| return nil, fmt.Errorf("unexpected git log output width %d columns", len(records)) | |
| return nil, elidedCommits, fmt.Errorf("unexpected git log output width %d columns", len(records)) | |
| } | |
| unixTS, err := strconv.ParseInt(records[1], 10, 64) | |
| if err != nil { | |
| @@ -280,6 +280,9 @@ | |
| for _, entry := range strings.Split(out, "\x00") { | |
| // Skip any trailing or consecutive NUL-separated empty entries | |
| if len(entry) == 0 { | |
| continue | |
| } | |
| records := strings.Split(entry, "\x1e") | |
| if len(records) != 4 { | |
| return nil, elidedCommits, fmt.Errorf("unexpected git log output width %d columns", len(records)) |
🤖 Prompt for AI Agents
In pkg/cli/admin/release/git.go around lines 280 to 286, the git log parsing
loop fails when git -z produces a trailing NUL because an empty entry yields the
wrong column count; modify the loop to skip empty entries (e.g., if entry == ""
continue) before splitting into records so the width check only runs on
non-empty entries, leaving the rest of the parsing logic unchanged.
There was a problem hiding this comment.
I'm not touching this null-splitting logic in this branch, I'm just adding elidedCommits to the return here to catch it up with the evolving function signature.
There was a problem hiding this comment.
@wking Understood! You're absolutely right to keep the scope focused on the elided commits functionality. The parsing robustness improvement can be addressed separately if needed. Thanks for clarifying the scope boundaries of this PR.
|
That would be nice to add an example how the page will look like when elidedCommits are found. |
|
This approach is far better than mine and looks good to me. |
openshift-ci
bot
added
the
do-not-merge/hold
| // but do at least mention the fact that there are non-merge commits in the | ||
| // first-parent line, so folks who are curious can click through to GitHub for | ||
| // details. | ||
| args := []string{"log", "--no-merges", "--first-parent", "-z", "--format=%H", fmt.Sprintf("%s..%s", from, to)} |
There was a problem hiding this comment.
I assume that this filter out the commits that we will print and only select the ones that are discarded?
…mmits Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this 'oc' repository: $ git log --graph --oneline -10 * 559c67e (HEAD -> main, origin/main) Merge pull request openshift#2083 from wking/oc-adm-upgrade-recommend-certificate-authority |\ | * b98b2e1 pkg/cli/admin/inspectalerts: Pass through --certificate-authority, etc. * | e7900fe Merge pull request openshift#2082 from ardaguclu/code-rabbit-test |\ \ | |/ |/| | * ffe752e Remove mentioning about IRC channel from README * | f114b17 OTA-1581: Promote `oc adm upgrade status` to general availability (openshift#2063) * | 1c54ec0 Merge pull request openshift#2077 from hongkailiu/OTA-1601 |\ \ | * | 5f1dbb8 Move not-lines-related table.Write out of the loop | * | a352662 Truncate long reasons | * | 11f9f47 Reproduce the long reason issue | * | 970f883 status: improve line break handling for CO Message 4299014 (Add squash-merge support into oc adm release info, 2022-04-26, openshift#1116), and mentioned the insights operator: $ git clone --depth 10 --branch master https://github.com/openshift/insights-operator $ cd insights-operator $ git log --graph --oneline -5 * da45333 (HEAD -> master, origin/master, origin/HEAD) OCPBUGS-60611: virt launcher logs gatherer (openshift#1110) * 737b59b fix: incorrect anonymization of domains (openshift#1111) * b5185da feat: add permissions to gather clusterrole (openshift#1106) * 3099eb0 feat: Allow on-demand gathering during initial periodic run (openshift#1109) * 7f8b40d RFE-4152: Add missing readonlyRootFilesystem (openshift#1104) But some repositories also use a mixture of real merges and squash or rebase merges: $ git clone --depth 20 --branch main https://github.com/openshift/operator-framework-operator-controller $ cd operator-framework-operator-controller $ git log --graph --oneline -11 * 9319f22a (HEAD -> main, origin/main, origin/HEAD) UPSTREAM: <carry>: Ensure unique name for bad-catalog tests * 9b50498a UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * 2ed1a5c8 UPSTREAM: <carry>: Migrate single/own namespace tests * 0c4f4303 UPSTREAM: <carry>: [OTE] add catalog tests from openshift/origin * 28299f38 UPSTREAM: <carry>: remove obsolete owners * 5aa7897f UPSTREAM: <carry>: [OTE] - Readme:Add info to help use payload-aggregate with new tests * 0bb19537 UPSTREAM: <carry>: Adds ResourceVersion checks to the tls secret deletion test, mirroring the logic used in the certificate rotation test. This makes the test more robust by ensuring a new secret is created, not just that an existing one is still present. * e9e3220f UPSTREAM: <carry>: [OTE] Add webhook to validate openshift-service-ca certificate rotation * a7d35272 Merge pull request openshift#453 from openshift-bot/synchronize-upstream |\ | * d93f9d16 UPSTREAM: <drop>: configure the commit-checker | * 9a69e8ed UPSTREAM: <drop>: remove upstream GitHub configuration Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes: $ git log --first-parent --merges --oneline a7d3527 Merge pull request openshift#453 from openshift-bot/synchronize-upstream 4cae159 Merge pull request openshift#449 from openshift-bot/synchronize-upstream 82f63dc Merge pull request openshift#444 from openshift-bot/synchronize-upstream 6f91d84 Merge pull request openshift#435 from openshift-bot/synchronize-upstream With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits: $ git log --first-parent --no-merges --oneline | wc -l 16 Folks interested in what exactly was elided will have to click through to 'Full changelog' or otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.
wking
force-pushed
the
adm-release-info-mention-elided-commits
branch
from
74dda80 to
977ac88
Compare
|
I just pushed 74dda80 -> 977ac88 to add a $ ./oc adm release info --changelog=/tmp/git registry.ci.openshift.org/ocp/release:4.20.0-0.nightly-2025-08-18-192426 registry.ci.openshift.org/ocp/release:4.20.0-0.nightly-2025-08-27-065636
...
### [olm-operator-controller](https://github.com/openshift/operator-framework-operator-controller/tree/9b50498a8a7d0a8d4a1f0cfc363151ba1dec506b)
* NO-ISSUE: Synchronize From Upstream Repositories [#453](https://github.com/openshift/operator-framework-operator-controller/pull/453)
* NO-ISSUE: Synchronize From Upstream Repositories [#449](https://github.com/openshift/operator-framework-operator-controller/pull/449)
* NO-ISSUE: Synchronize From Upstream Repositories [#444](https://github.com/openshift/operator-framework-operator-controller/pull/444)
* And 13 elided commits (e.g. from squash or rebase merges)
* [Full changelog](https://github.com/openshift/operator-framework-operator-controller/compare/1a9f810a3338d1d31b4a9a3d63a1390c67a2a8f7...9b50498a8a7d0a8d4a1f0cfc363151ba1dec506b)
...Which looks good to me. /hold cancel I'll still need to collect |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
For fun, here's poking at the JSON output for those two nightlies: $ ./oc adm release info --changelog=/tmp/git -o json registry.ci.openshift.org/ocp/release:4.20.0-0.nightly-2025-08-18-192426 registry.ci.openshift.org/ocp/release:4.20.0-0.nightly-2025-08-27-065636 | jq -c '.updatedImages[] | select(.elidedCommits) | {elidedCommits, commits: (.commits | length), name, fullChangeLog}'
{"elidedCommits":1,"commits":1,"name":"cli, cli-artifacts, deployer, tools","fullChangeLog":"https://github.com/openshift/oc/compare/1c54ec07271de9a954826ae08d9b1b1c423fe0f2...e7900febdfbef1ab88713a85e4f0b16d0f886566"}
{"elidedCommits":3,"commits":15,"name":"console","fullChangeLog":"https://github.com/openshift/console/compare/9f4a18aaed3f9c71365d2fff91e2dc515bafbc5f...2151cf2dcc41fa8ca3af2c038ea1102ac6046832"}
{"elidedCommits":13,"commits":2,"name":"olm-catalogd","fullChangeLog":"https://github.com/openshift/operator-framework-operator-controller/compare/82f63dc73b08b02294a04cd2105bad8fc776c2b6...9b50498a8a7d0a8d4a1f0cfc363151ba1dec506b"}
{"elidedCommits":13,"commits":3,"name":"olm-operator-controller","fullChangeLog":"https://github.com/openshift/operator-framework-operator-controller/compare/1a9f810a3338d1d31b4a9a3d63a1390c67a2a8f7...9b50498a8a7d0a8d4a1f0cfc363151ba1dec506b"}An elided commit in $ git log --first-parent --no-merges --oneline 1c54ec07271de9a954826ae08d9b1b1c423fe0f2..e7900febdfbef1ab88713a85e4f0b16d0f886566
f114b178f OTA-1581: Promote `oc adm upgrade status` to general availability (#2063)Ah, a squash merge. |
So even in oc, we may not show some commits. Interesting. |
I'm wondering why didn't this commit 4299014 catch this commit in oc? |
|
Since oc/pkg/cli/admin/release/git.go Line 246 in 559c67e This PR fixes an issue bigger than I anticipated. |
|
I think, this is a safe change to merge. Also in worst case scenario, it is not likely that it affects customers |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
/retest-required |
|
/retest-required |
This pull takes us from "silently ignore squash/rebase first-parent entries when there are real merge commits" to "mention a count of elided squash/rebase first-parent entries when there are real merge commits". It would be even nicer if we didn't switch so hard on "when there are real merge commits", and instead had logic that iterated over all the first-parent commits, used the merge-commit parser when the commit was a merge, used the squash parser when the commit was not a merge, and incremented the elided count when squash parser failed (e.g. because it was a rebase merge or whatever). I haven't gone that far, but it would be an interesting follow-up project if anyone's feeling sufficiently motivated. |
|
This time image registry had 18s disruption, but a retest is already running. |
|
@wking: 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. |
openshift-merge-bot
bot
merged commit 31597ed
into
openshift:main
|
@wking: Jira Issue OCPBUGS-60989: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-60989 has been moved to the MODIFIED state. 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. |
|
I think, in order to changes are effective, oc needs to be updated |
3 participants
Originally, the change-log generation focused on merge commits, which are common in many repositories. For example, in this
ocrepository:4299014 (#1116), and mentioned the insights operator:
But some repositories also use a mixture of real merges and squash or rebase merges:
Before this commit, the logic would find the merge commits, assume that all content came in via merge commits, and not mention the other changes:
With this commit, we'll continue to include a line for each of those merges. But we'll also include the count of any elided, non-merge, first-parent commits:
Folks interested in what exactly was elided will have to click through to
Full changelogor otherwise go digging. But at least they'll get the count of elided commits to know that there is more information there to see behind that click.Summary by CodeRabbit
New Features
Tests