github: commit_linter: Make more flexibility for commit linter #11251
Conversation
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
WalkthroughThe commit-prefix checker now returns (prefixes, build_optional) from path inference and applies more nuanced validation: component prefixes (lib:, tests:, plugins/:, src/... or dir-specific:, build:) are detected; build: can be optional; subject prefix must match inferred prefixes with special umbrella/build handling. (49 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/tests/test_commit_lint.py (1)
607-623: Test assertion contradicts its docstring.Similar to the earlier test, the docstring at lines 609-613 claims "This test used to require an error, but now it verifies that such a commit passes validation." However, the assertion at line 620 expects
ok is False.This is inconsistent. Please align the docstring with the actual expected behavior.
def test_error_multiple_prefixes_one_matches(): """ - Under the new rules, touching multiple components is allowed if the - subject prefix matches any inferred component. - - This test used to require an error, but now it verifies that such a - commit passes validation. + Commits touching multiple non-build components are rejected even if the + subject prefix matches one of them. Users must split into separate commits. """ commit = make_commit( "in_tail: update\n\nSigned-off-by: User", ["plugins/in_tail/tail.c", "src/flb_router.c"] ) ok, msg = validate_commit(commit) assert ok is False assert "Expected one of:" in msg assert "in_tail:" in msg assert "router:" in msg
🧹 Nitpick comments (1)
.github/scripts/commit_prefix_check.py (1)
204-204: Minor: Extra blank line.There's a redundant blank line at line 204. Consider removing it for consistency.
) - # If build is NOT optional and build: exists among expected,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/commit_prefix_check.py(4 hunks).github/scripts/tests/test_commit_lint.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/tests/test_commit_lint.py (1)
.github/scripts/commit_prefix_check.py (2)
infer_prefix_from_paths(30-85)validate_commit(123-213)
🪛 Ruff (0.14.7)
.github/scripts/tests/test_commit_lint.py
262-262: Unpacked variable msg is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (3)
.github/scripts/commit_prefix_check.py (3)
30-85: LGTM!The refactored
infer_prefix_from_pathsfunction clearly separates component prefixes from build-related prefixes and correctly computesbuild_optional. The logic for inferring prefixes from various path patterns (lib/, tests/, plugins/, src/) is well-structured and handles edge cases like Windows path separators.
170-193: LGTM on multi-component detection logic.The logic correctly rejects commits touching multiple non-build components unless the subject uses an umbrella prefix like
lib:. This enforces the "one component per commit" rule while allowing flexibility for library-wide changes.The implementation aligns with the test assertions (which expect rejection), though as noted in the test file review, the test docstrings need to be corrected to match this behavior.
219-269: LGTM!The
get_pr_commitsfunction correctly handles various CI scenarios (pull_request vs other events), attempts multiple fallback strategies for finding the base branch, and properly filters out merge commits. Good defensive programming with try/except blocks.
cosmo0920
force-pushed
the
cosmo0920-make-more-flexible-for-commit-linter
branch
from
dd5f324 to
eba28b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/scripts/commit_prefix_check.py (1)
28-85: Consider tightening src/ top-level handling and removing dead CMakeLists.txt special-caseThe
src/inference still usesparts[1]as the prefix, which means a top-level file likesrc/foo.cwould inferfoo.c:instead offoo:. If such files exist, commits using the naturalfoo:prefix will be rejected. You can normalize this by stripping the extension when there is no subdirectory:- if p.startswith("src/"): - filename = os.path.basename(p) - if filename.startswith("flb_"): - core = filename[4:].split(".")[0] - component_prefixes.add(f"{core}:") - else: - parts = p.split("/") - if len(parts) > 1: - component_prefixes.add(f"{parts[1]}:") + if p.startswith("src/"): + filename = os.path.basename(p) + if filename.startswith("flb_"): + core = filename[4:].split(".")[0] + component_prefixes.add(f"{core}:") + else: + parts = p.split("/") + if len(parts) > 1: + name = parts[1] + # Handle both src/<dir>/... and src/<file> cases + component_prefixes.add(f"{os.path.splitext(name)[0]}:")Also,
cmakelists.txt:is never produced byinfer_prefix_from_paths(onlybuild:is), so the special-casing of"cmakelists.txt:"in later logic appears dead and could be dropped for clarity unless you are preserving it for historical reasons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/commit_prefix_check.py(4 hunks).github/scripts/tests/test_commit_lint.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/tests/test_commit_lint.py (1)
.github/scripts/commit_prefix_check.py (2)
infer_prefix_from_paths(30-85)validate_commit(123-213)
🔇 Additional comments (4)
.github/scripts/commit_prefix_check.py (1)
121-212: Multi-component + build_optional rules look consistent with testsThe updated
validate_commitflow (bad-squash handling, subject length, signoff check,(expected, build_optional)usage, multi-component rejection withlib:as umbrella, and strictbuild:requirement when only build-system files are touched) is coherent and matches the new test expectations for mixed components, build-only changes, and multi-signoff scenarios. No functional issues spotted here..github/scripts/tests/test_commit_lint.py (3)
30-68: Inference tests correctly cover plugins, core files, and multi-component setsThe updated prefix inference tests for plugins and core (
plugins/out_s3,src/flb_*, and mixed plugin+core) now assert bothprefixesandbuild_optional, and match the current behavior ofinfer_prefix_from_paths(real components implybuild_optional is True). This looks solid and provides good coverage for the common cases.
250-262: Multi-component rejection test now matches the intended rulesThe docstring and assertions in
test_error_multiple_prefixes_inferred_from_filesare now aligned: touchingplugins/in_tailandsrc/flb_router.cwith a singlein_tail:prefix is expected to fail, and the test checks for the generic "does not match files changed" wording. This accurately exercises the new rule that multi non-build components must be split.
290-353: Edge-case inference tests nicely pin down build_optional semanticsThe additional
infer_prefix_from_pathstests for empty lists,src/subdirectories, unknown paths, repeated plugin files, and mixed known/unknown paths all correctly validate:
prefixes == set()andbuild_optional is Falsewhen nothing or only unknown paths are seen.- Concrete component prefixes (
stream_processor:,out_s3:,out_http:,http2:,in_tail:) andbuild_optional is Truewhen at least one real component is touched.These give good confidence that the new
(prefixes, build_optional)contract is stable across edge cases.
Signed-off-by: Hiroshi Hatake <[email protected]>
cosmo0920
force-pushed
the
cosmo0920-make-more-flexible-for-commit-linter
branch
from
eba28b3 to
79d9c6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/scripts/commit_prefix_check.py (1)
30-85: Clarifybuild_optionalcontract in docstring vs non-build callersThe implementation and usage are sound, but the docstring for
infer_prefix_from_paths(Lines 31–36) suggestsbuild_optionalis onlyFalse“when modifying build system files alone”. In practice you also returnbuild_optional is Falsewhen there are no inferred prefixes at all (e.g., empty or docs/CI file lists), even thoughvalidate_commitwill then short‑circuit onlen(expected) == 0and never look atbuild_optional.To avoid confusion for future maintainers, consider tightening the docstring to note that:
build_optionalis only meaningful when"build:"is present inprefixes, and- For non‑build changes where no
build:prefix is inferred, its value is a don’t‑care (currentlyFalse).This keeps the behavior unchanged but better documents the invariant actually relied on by
validate_commit..github/scripts/tests/test_commit_lint.py (1)
30-68: Inference tests accurately cover tuple return; consider adding build‑mixed casesThe updated prefix inference tests (Lines 30–68 and 290–353) look aligned with the new
infer_prefix_from_pathscontract:
- Plugin and core file paths map to the expected prefixes.
- Multiple components yield multiple prefixes.
build_optionalis assertedTruewhenever at least one component is inferred, andFalsefor empty/unknown paths.Given how
build_optionalis used invalidate_commit, it would be a nice follow‑up to add a couple of focused cases:
- A pure build change (
["CMakeLists.txt"]) expecting({"build:"}, False).- A mixed component+build change (
["src/flb_router.c", "CMakeLists.txt"]) expecting({"router:", "build:"}, True).This would lock in the intended semantics around when
build:is mandatory vs optional without changing the current behavior.Also applies to: 290-353
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/scripts/commit_prefix_check.py(4 hunks).github/scripts/tests/test_commit_lint.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.github/scripts/tests/test_commit_lint.py (1)
.github/scripts/commit_prefix_check.py (2)
infer_prefix_from_paths(30-85)validate_commit(123-213)
🔇 Additional comments (2)
.github/scripts/commit_prefix_check.py (1)
120-213: Multi‑component rejection and build‑only handling look correctThe updated
validate_commitlogic around Lines 160–213 correctly implements the intended rules:
- Any subject prefix not matching
PREFIX_REis rejected early.detect_bad_squashresults are only fatal when due to prefix‑like lines in the body, while multiple sign‑offs are ignored here (but still flagged by the helper), matching the tests.non_build_prefixesandumbrella_prefixesensure:
- Pure build changes (
expected == {"build:"}withbuild_optional is False) must usebuild:in the subject.- Commits touching multiple non‑build components (e.g., plugin + core) are rejected even if the subject matches one of them, as exercised by
test_error_multiple_prefixes_inferred_from_filesandtest_error_multiple_prefixes_one_matches.- Umbrella prefixes like
lib:can span multiple inferred prefixes when present.Given the current test suite, this behavior is consistent and I don’t see correctness issues here.
.github/scripts/tests/test_commit_lint.py (1)
250-262: Multi‑component validation tests now match intended behaviorThe docstrings and assertions for
test_error_multiple_prefixes_inferred_from_filesandtest_error_multiple_prefixes_one_matches(Lines 250–262 and 605–613) are now consistent with the new rules:
- Commits touching multiple non‑build components are explicitly expected to be rejected, even when the subject prefix matches one inferred component.
- The tests verify both the failure (
ok is False) and that the error message includesExpected one of:and the relevant component prefixes.This aligns cleanly with the
non_build_prefixeslogic invalidate_commitand removes the earlier docstring/behavior mismatch.Also applies to: 605-613
patrick-stephens
left a comment
patrick-stephens
left a comment
There was a problem hiding this comment.
Should it be workflows though prefix?
Yes, it works. |
This patch refines the commit prefix linter used in CI so that it:
infers prefixes more accurately from the modified paths (including lib/, tests/, plugins/, src/ and build files),
rejects commits that mix multiple components in a single change,
keeps “bad squash” detection strict, while still allowing legitimate multiple Signed-off-by trailers, and fixes several false positives that the current linter reports for existing history.
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
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.