build: Detect version bump to permit "release: " prefix#11638
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
📝 WalkthroughWalkthroughThe changes add validation logic to ensure commits modifying version information in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e29e2b3161
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/scripts/tests/test_commit_lint.py (2)
926-936: Consider addinghexshafor robustness.
FakeCommitis missing thehexshaattribute whichvalidate_commituses in theGitCommandErrorhandling (line 264 in the main file). While current tests won't trigger that path, adding it would make the fake more complete and prevent potentialAttributeErrorif error handling paths are tested later.♻️ Suggested addition
class FakeCommit: def __init__(self, message, diffs): self.message = message self._diffs = diffs self.parents = [object()] + self.hexsha = "deadbeef1234" file_paths = [d.b_path for d in diffs] self.stats = FakeStats(file_paths)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/tests/test_commit_lint.py around lines 926 - 936, FakeCommit used in tests lacks the hexsha attribute that validate_commit references when handling GitCommandError; add a hexsha field to FakeCommit (e.g., self.hexsha = some stable dummy value or derived from the message) so error-handling paths that access commit.hexsha won’t raise AttributeError; update the FakeCommit __init__ to set self.hexsha while keeping existing behavior of message, _diffs, parents, and stats.
964-977: Consider asserting the error message for clarity.The test passes, but the rejection reason differs from what one might expect. When
CMakeLists.txthas non-version changes,is_version_bump()returnsFalse, and normal validation fails becauserelease:is not in the expected prefixes (onlybuild:is inferred).Adding an assertion on the error message would document the expected behavior and catch regressions:
♻️ Suggested improvement
- ok, _ = validate_commit(commit) + ok, msg = validate_commit(commit) assert ok is False + assert "does not match files changed" in msg or "Expected one of: build:" in msgAlso, consider adding a test for a pure version bump with the wrong prefix (e.g.,
build:instead ofrelease:):def test_version_bump_requires_release_prefix(): """Version bumps must use the release: prefix, not other prefixes.""" commit = make_fake_commit( "build: bump version to 5.0.2\n\nSigned-off-by: User", [("CMakeLists.txt", "-set(FLB_VERSION_PATCH 0)\n+set(FLB_VERSION_PATCH 2)\n")] ) ok, msg = validate_commit(commit) assert ok is False assert "Version bump must use release: prefix" in msg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/tests/test_commit_lint.py around lines 964 - 977, Update the test to assert the expected rejection message so behavior is explicit: in test_release_rejected_when_cmakelists_has_non_version_change, after calling validate_commit(commit) assert ok is False and also assert the returned message contains the expected text (use the actual rejection string returned by validate_commit/is_version_bump logic). Additionally add the suggested new test (test_version_bump_requires_release_prefix) that constructs a pure version change but with the wrong prefix and asserts validate_commit returns False and the message contains the "Version bump must use release: prefix" text; reference validate_commit and is_version_bump to locate the validation paths to confirm the exact message to assert against..github/scripts/commit_prefix_check.py (1)
193-220: Consider limiting version detection to root CMakeLists.txt only.The check
path.endswith("CMakeLists.txt")matches any CMakeLists.txt in the repository (root,plugins/*/CMakeLists.txt,lib/*/CMakeLists.txt, etc.). If a release commit updates the version in root CMakeLists.txt but also touches a subdirectory CMakeLists.txt for a legitimate reason (e.g., updating a dependency version),is_version_bump()would returnFalseand the commit would fail normal validation becauserelease:is not an inferred prefix.If this is intentional (release commits should only touch root version numbers), consider adding a comment explaining this constraint. Otherwise, restrict to the root file:
♻️ Proposed fix to restrict to root CMakeLists.txt
- if not path.endswith("CMakeLists.txt"): + if path != "CMakeLists.txt": continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/commit_prefix_check.py around lines 193 - 220, The current check treats any CMakeLists.txt as the root version file (uses path.endswith("CMakeLists.txt")), which causes is_version_bump() to fail when submodule or plugin CMake files are changed; update the condition to only consider the repository root file (e.g., require path == "CMakeLists.txt" or normalize and compare to the exact root path) in the loop that processes diffs, and add a short comment in the is_version_bump() function explaining that only the root CMakeLists.txt is checked (or keep the existing behavior but document it) so future readers understand the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/scripts/commit_prefix_check.py:
- Around line 193-220: The current check treats any CMakeLists.txt as the root
version file (uses path.endswith("CMakeLists.txt")), which causes
is_version_bump() to fail when submodule or plugin CMake files are changed;
update the condition to only consider the repository root file (e.g., require
path == "CMakeLists.txt" or normalize and compare to the exact root path) in the
loop that processes diffs, and add a short comment in the is_version_bump()
function explaining that only the root CMakeLists.txt is checked (or keep the
existing behavior but document it) so future readers understand the constraint.
In @.github/scripts/tests/test_commit_lint.py:
- Around line 926-936: FakeCommit used in tests lacks the hexsha attribute that
validate_commit references when handling GitCommandError; add a hexsha field to
FakeCommit (e.g., self.hexsha = some stable dummy value or derived from the
message) so error-handling paths that access commit.hexsha won’t raise
AttributeError; update the FakeCommit __init__ to set self.hexsha while keeping
existing behavior of message, _diffs, parents, and stats.
- Around line 964-977: Update the test to assert the expected rejection message
so behavior is explicit: in
test_release_rejected_when_cmakelists_has_non_version_change, after calling
validate_commit(commit) assert ok is False and also assert the returned message
contains the expected text (use the actual rejection string returned by
validate_commit/is_version_bump logic). Additionally add the suggested new test
(test_version_bump_requires_release_prefix) that constructs a pure version
change but with the wrong prefix and asserts validate_commit returns False and
the message contains the "Version bump must use release: prefix" text; reference
validate_commit and is_version_bump to locate the validation paths to confirm
the exact message to assert against.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4abadb67-1009-448a-b0fc-23bac808fe25
📒 Files selected for processing (2)
.github/scripts/commit_prefix_check.py.github/scripts/tests/test_commit_lint.py
2 participants
Version bumping process that we always use needs to use
release:prefix but this rule is not implemented in our linter. So, we need to handle this.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
release:prefix in the commit message and cannot include unrelated changes.