-
Notifications
You must be signed in to change notification settings - Fork 1.9k
github: scripts: commit_prefix_check: Add tests into umbrella rule #11407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
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.