github: scripts: commit_prefix_check: Add tests into umbrella rule#11407
Conversation
…bject Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
📝 WalkthroughWalkthroughCommit prefix inference now derives specific prefixes from test file names (stripping leading Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b6eb37f9c
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 0
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)
606-618: Docstring is now outdated.The docstring states that "Test files (in tests/ directory) don't generate specific component prefixes" and "Generic prefixes like 'tests:' are acceptable." With the new logic, test files do generate specific prefixes (e.g.,
test_router:fromtest_router.c), and the test message was updated to reflect the nested prefix style.📝 Suggested docstring update
def test_valid_test_file_changes(): """ - Test that test file changes are allowed with generic prefixes. + Test that test file changes allow umbrella prefix with nested scope. - Test files (in tests/ directory) don't generate specific component prefixes. - Generic prefixes like 'tests:' are acceptable for test-related changes. + Test files (in tests/ directory) generate both specific prefixes (derived + from filename) and the umbrella 'tests:' prefix. Commits can use the + 'tests:' umbrella prefix with a nested scope (e.g., 'tests: test_router:'). """
🧹 Nitpick comments (1)
.github/scripts/commit_prefix_check.py (1)
252-268: Consider consolidating duplicate validation blocks.The
lib:andtests:umbrella validation blocks are nearly identical, differing only in the directory prefix check. This could be consolidated using a mapping.♻️ Suggested refactor
if len(non_build_prefixes) > 1: if subj_lower in umbrella_prefixes: norm_paths = [p.replace(os.sep, "/") for p in files] - if subj_lower == "lib:": - if not all(p.startswith("lib/") for p in norm_paths): - expected_list = sorted(expected) - expected_str = ", ".join(expected_list) - return False, ( - f"Subject prefix '{subject_prefix}' does not match files changed.\n" - f"Expected one of: {expected_str}" - ) - - elif subj_lower == "tests:": - if not all(p.startswith("tests/") for p in norm_paths): - expected_list = sorted(expected) - expected_str = ", ".join(expected_list) - return False, ( - f"Subject prefix '{subject_prefix}' does not match files changed.\n" - f"Expected one of: {expected_str}" - ) + umbrella_to_dir = {"lib:": "lib/", "tests:": "tests/"} + required_dir = umbrella_to_dir[subj_lower] + if not all(p.startswith(required_dir) for p in norm_paths): + expected_list = sorted(expected) + expected_str = ", ".join(expected_list) + return False, ( + f"Subject prefix '{subject_prefix}' does not match files changed.\n" + f"Expected one of: {expected_str}" + )
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
|
Apart from my hatred of python this looks fine :) |
| f"Subject prefix '{subject_prefix}' does not match files changed.\n" | ||
| f"Expected one of: {expected_str}" | ||
| ) | ||
| elif subj_lower not in umbrella_prefixes: |
There was a problem hiding this comment.
Removal of this line seems to contradict the concept of umbrella prefix, e.g. I failed to find correct prefix in #7608 for f897b94 commit which includes following files:
- include/fluent-bit/flb_lib.h
- include/fluent-bit/flb_output.h
- src/flb_engine_dispatch.c
- src/flb_lib.c
I use lib: prefix ("lib: support of tests which require callback context depending on configuration" commit message), but this script complains:
❌ Commit f897b943a0 failed: Subject prefix 'lib:' does not match files changed. Expected one of: engine_dispatch:, lib:
We need to permit this kind of style of commit messages:
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.