feat: support merge-base option
#86
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request enhance the Commit Check GitHub Action by adding a new optional input parameter, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Commit-Check ✔️ |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
.github/workflows/commit-check.yml (1)
27-27: Add inline documentation for the merge-base parameterThe purpose and behavior of this new parameter should be documented in the workflow file for better maintainability.
Add a comment explaining the parameter:
+ # Validates if the current branch is properly rebased onto the target branch merge-base: true.commit-check.yml (2)
26-26: Remove unnecessary empty lineThe empty line between checks affects readability and isn't consistent with the spacing between other checks.
suggest: run command `git config user.email yourname@example.com` - - check: merge_base
29-30: Improve error and suggestion messages with more specific guidanceThe current messages could be more helpful by including specific commands and the target branch name.
- error: Current branch is not rebased onto target branch - suggest: please ensure your branch is rebased with the target branch + error: Current branch is not rebased onto the target branch (${TARGET_BRANCH}) + suggest: Run these commands to rebase:\n git fetch origin ${TARGET_BRANCH}\n git rebase origin/${TARGET_BRANCH}action.yml (1)
Line range hint
52-61: Critical: Missing environment variable for merge-base inputThe
merge-baseinput parameter needs to be passed to the Python script through an environment variable, similar to other inputs.Add the missing environment variable mapping:
env: MESSAGE: ${{ inputs.message }} BRANCH: ${{ inputs.branch }} AUTHOR_NAME: ${{ inputs.author-name }} AUTHOR_EMAIL: ${{ inputs.author-email }} COMMIT_SIGNOFF: ${{ inputs.commit-signoff }} DRY_RUN: ${{ inputs.dry-run }} JOB_SUMMARY: ${{ inputs.job-summary }} PR_COMMENTS: ${{ inputs.pr-comments }} + MERGE_BASE: ${{ inputs.merge-base }}main.py (1)
Line range hint
41-63: Consider adding docstring for merge-base functionalityWhile the implementation is solid, it would be helpful to add documentation explaining:
- The purpose of the merge-base check
- Expected values and behavior
- Impact on the overall commit check process
Add a docstring to explain the merge-base functionality:
def run_commit_check() -> int: - """Runs the commit-check command and logs the result.""" + """Runs the commit-check command and logs the result. + + The following checks are performed: + - Message format validation + - Branch naming convention + - Author name and email verification + - Commit sign-off requirement + - Merge base validation (ensures current branch is rebased onto target branch) + + Returns: + int: The return code from the commit-check command execution + """README.md (2)
76-82: Improve documentation clarity and fix grammar issues.The documentation for the new
merge-baseparameter needs some improvements:
- Fix grammar: "disable" should be "disabled"
- Fix capitalization: "by default" should be "By default"
- Add more context about what "rebased onto target branch" means and its implications
Apply this diff:
### `merge-base` - **Description**: check current branch is rebased onto target branch. - Default: 'false' > [!IMPORTANT] -> `merge-base` is an experimental feature. by default it's disable. +> `merge-base` is an experimental feature. By default it's disabled. When enabled, it ensures your branch contains all commits from the target branch, helping prevent merge conflicts and maintain a clean git history.🧰 Tools
🪛 LanguageTool
[uncategorized] ~82-~82: A comma is probably missing here.
Context: ...ge-baseis an experimental feature. by default it's disable. ###dry-run` - **Descr...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
100-102: Fix grammar and capitalization in the important notice.The notice about the experimental
pr-commentsfeature needs some improvements:Apply this diff:
> [!IMPORTANT] -> `pr-comments` is an experimental feature. by default it's disable. To use it you need to set `GITHUB_TOKEN` in the GitHub Action. +> `pr-comments` is an experimental feature. By default it's disabled. To use it, you need to set `GITHUB_TOKEN` in the GitHub Action.🧰 Tools
🪛 LanguageTool
[uncategorized] ~100-~100: A comma is probably missing here.
Context: ...omments` is an experimental feature. by default it's disable. To use it you need to set...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[typographical] ~100-~100: It seems that a comma is missing.
Context: ...eature. by default it's disable. To use it you need to setGITHUB_TOKENin the G...(IN_ORDER_TO_VB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.commit-check.yml(1 hunks).github/workflows/commit-check.yml(1 hunks)README.md(2 hunks)action.yml(1 hunks)main.py(3 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~82-~82: A comma is probably missing here.
Context: ...ge-baseis an experimental feature. by default it's disable. ###dry-run` - **Descr...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[uncategorized] ~100-~100: A comma is probably missing here.
Context: ...omments` is an experimental feature. by default it's disable. To use it you need to set...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[typographical] ~100-~100: It seems that a comma is missing.
Context: ...eature. by default it's disable. To use it you need to set GITHUB_TOKEN in the G...
(IN_ORDER_TO_VB_COMMA)
🔇 Additional comments (8)
.github/workflows/commit-check.yml (1)
27-27: 🛠️ Refactor suggestion
Inconsistent parameter naming across files
The parameter uses hyphenated format (merge-base) in workflow but the AI summary indicates underscore format (merge_base) in implementation. This inconsistency could lead to confusion.
Let's verify the naming convention across files:
#!/bin/bash
# Description: Check naming consistency
# Expected: Should show all occurrences of both merge-base and merge_base
# Search for both naming conventions
echo "Hyphenated format:"
rg "merge-base"
echo -e "\nUnderscore format:"
rg "merge_base"Standardize the naming convention across all files to either:
- Hyphenated format:
merge-base(GitHub Actions convention) - Underscore format:
merge_base(Python convention)
.commit-check.yml (1)
27-30: Verify integration with action implementation
The merge-base check configuration looks good, but let's verify its integration with the action implementation.
action.yml (1)
28-31: LGTM: Well-structured input parameter definition
The new merge-base input parameter is properly defined with clear description, required flag, and default value, following the same pattern as other input parameters.
main.py (3)
19-19: LGTM: Environment variable declaration follows established patterns
The new MERGE_BASE environment variable is correctly declared with appropriate default value, maintaining consistency with other boolean flags in the codebase.
36-36: LGTM: Logging implementation is consistent
The logging of MERGE_BASE follows the established pattern and is properly placed within the log_env_vars function.
50-56: Verify commit-check CLI support for --merge-base flag
The implementation correctly adds the merge-base option to the command arguments. However, we should verify that the commit-check command supports this new flag.
README.md (2)
42-42: LGTM! Example configuration is clear and well-placed.
The new merge-base parameter is appropriately added to the example configuration with a safe default value of false.
63-63: LGTM! Parameter descriptions are consistent and clear.
The updated descriptions maintain a consistent style with proper punctuation.
Also applies to: 68-68, 73-73, 91-91, 96-96
3d0d663 to
28387a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
README.md (2)
76-84: Fix grammatical issues in merge-base documentationThere are several grammatical issues in the documentation that should be addressed.
- > `merge-base` is an experimental feature. by default it's disable. - > To use this feature, you need fetch all history for all branches by setting `fetch-depth: 0` in `actions/checkout`. + > `merge-base` is an experimental feature. By default, it's disabled. + > To use this feature, you need to fetch all history for all branches by setting `fetch-depth: 0` in `actions/checkout`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~82-~82: A comma is probably missing here.
Context: ...ge-base` is an experimental feature. by default it's disable. > > To use this feature,...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[grammar] ~84-~84: The verb “fetch” needs to be in the to-infinitive form.
Context: ... disable. > > To use this feature, you need fetch all history for all branches by setting...(MISSING_TO_BEFORE_A_VERB)
102-104: Fix grammatical issues in PR comments documentationSimilar grammatical issues need to be addressed in the PR comments section.
- > `pr-comments` is an experimental feature. by default it's disable. To use it you need to set `GITHUB_TOKEN` in the GitHub Action. + > `pr-comments` is an experimental feature. By default, it's disabled. To use it, you need to set `GITHUB_TOKEN` in the GitHub Action.🧰 Tools
🪛 LanguageTool
[uncategorized] ~102-~102: A comma is probably missing here.
Context: ...omments` is an experimental feature. by default it's disable. To use it you need to set...(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[typographical] ~102-~102: It seems that a comma is missing.
Context: ...eature. by default it's disable. To use it you need to setGITHUB_TOKENin the G...(IN_ORDER_TO_VB_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.commit-check.yml(1 hunks).github/workflows/commit-check.yml(2 hunks)README.md(2 hunks)action.yml(2 hunks)main.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .commit-check.yml
- action.yml
- main.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~82-~82: A comma is probably missing here.
Context: ...ge-base` is an experimental feature. by default it's disable. > > To use this feature,...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[grammar] ~84-~84: The verb “fetch” needs to be in the to-infinitive form.
Context: ... disable. > > To use this feature, you need fetch all history for all branches by setting...
(MISSING_TO_BEFORE_A_VERB)
[uncategorized] ~102-~102: A comma is probably missing here.
Context: ...omments` is an experimental feature. by default it's disable. To use it you need to set...
(MISSING_COMMA_AFTER_INTRODUCTORY_PHRASE)
[typographical] ~102-~102: It seems that a comma is missing.
Context: ...eature. by default it's disable. To use it you need to set GITHUB_TOKEN in the G...
(IN_ORDER_TO_VB_COMMA)
🔇 Additional comments (4)
.github/workflows/commit-check.yml (2)
18-18: LGTM: Required for merge-base functionality
Setting fetch-depth: 0 is necessary to fetch the complete git history, which is required for determining the merge-base between branches.
28-28: Consider the implications of enabling experimental feature in self-test
The previous review comment about the experimental status of merge-base is still valid.
#!/bin/bash
# Description: Verify merge-base parameter configuration
# Expected: Should show merge-base marked as experimental with default: false
# Check action.yml for merge-base input configuration
rg -A 5 "merge-base:" action.ymlREADME.md (2)
63-63: LGTM: Documentation style improvements
The updates maintain consistency by ensuring all parameter descriptions end with periods.
Also applies to: 68-68, 73-73
93-93: LGTM: Documentation style improvements
The updates maintain consistency by ensuring all parameter descriptions end with periods.
Also applies to: 98-98
closes #85
related to commit-check/commit-check#191
Summary by CodeRabbit
New Features
merge-basefor checking if the current branch is rebased onto the target branch.merge-baseparameter for improved functionality.Documentation
README.mdto include details about the newmerge-baseparameter and its default state.Bug Fixes