fix: fixed the notify triager workflow #3460
Conversation
sambhavgupta0705
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
|
Caution Review failedThe pull request is closed. WalkthroughThe GitHub Actions workflow file Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Tested the workflow here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3460 +/- ##
=======================================
Coverage 86.59% 86.59%
=======================================
Files 21 21
Lines 664 664
=======================================
Hits 575 575
Misses 89 89 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3460--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/notify-triager.yml (4)
20-23: Consider making the commit message extraction more robustThe current implementation might:
- Remove legitimate characters from commit messages (e.g., brackets in conventional commit scopes)
- Lose important formatting due to aggressive sanitization
Consider this more selective sanitization:
- commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | sed 's/[][]//g' | sed 's/(//g' | sed 's/)//g' | xargs) + # Only remove characters that could cause issues in GitHub API calls + commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | tr '\n' ' ' | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
46-46: Remove outdated version commentThe inline comment references version 42.1.0 but the action is using v45.
- uses: tj-actions/changed-files@v45 # version 42.1.0 https://github.com/tj-actions/changed-files/releases/tag/v42.1.0 + uses: tj-actions/changed-files@v45
50-50: Fix formatting issuesRemove the trailing whitespace and empty lines.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 50-50: trailing spaces
(trailing-spaces)
100-100: Fix file formattingAdd a newline at the end of the file and remove trailing whitespace.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 100-100: no new line character at the end of file
(new-line-at-end-of-file)
[error] 100-100: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/notify-triager.yml(5 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/notify-triager.yml
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 100-100: no new line character at the end of file
(new-line-at-end-of-file)
[error] 100-100: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/notify-triager.yml (3)
40-43: LGTM! Good practice using a pinned version.
The addition of this step with a specific version tag improves stability and reliability.
87-87: LGTM! Consistent with the workflow's purpose.
The condition correctly triggers doc reviewer notifications when markdown files are changed.
72-72: LGTM! This fixes the core issue.
The condition correctly prevents code reviewer notifications when only markdown files are changed by comparing the total changed files count with markdown files count.
Let's verify the logic works as expected:
✅ Verification successful
Logic verification confirms this is a correct implementation
The workflow implementation correctly handles the core requirement:
- Uses
changed-filesaction twice:- First to get all changed files count (
steps.changed-files.outputs.all_changed_files_count) - Second to get markdown files count (
steps.md-pr-changes.outputs.all_changed_files_count) with**.mdfilter
- First to get all changed files count (
- The condition
steps.changed-files.outputs.all_changed_files_count != steps.md-pr-changes.outputs.all_changed_files_countwill:- Be true when there are non-markdown changes (triggering code reviewer notification)
- Be false when all changes are markdown files (skipping code reviewer notification)
- The merge commit check (
steps.check-merge-branch.outputs.isMergeCommit == 'false') properly prevents notifications for merge commits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test the condition with different file change scenarios
# Create test PR with only MD changes
cat << EOF > test_changes.json
{
"md_only": {
"changed_files": ["docs/a.md", "docs/b.md"],
"expected": "should_not_notify_code_reviewers"
},
"mixed": {
"changed_files": ["docs/a.md", "src/code.js"],
"expected": "should_notify_code_reviewers"
}
}
EOF
# Verify the workflow handles these cases
gh workflow run notify-triager.yml --json test_changes.json
Length of output: 244
Script:
#!/bin/bash
# Let's verify the workflow logic by examining the actual implementation
# First, check the workflow file content
cat .github/workflows/notify-triager.yml
# Then, let's examine the steps that provide the outputs used in the condition
rg -A 2 "id: changed-files" .github/workflows/notify-triager.yml
rg -A 2 "id: md-pr-changes" .github/workflows/notify-triager.yml
Length of output: 4347
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/notify-triager.yml (3)
20-23: Consider optimizing the commit message sanitization.The current implementation works but could be more efficient. Consider combining the multiple sed commands into a single operation.
- commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | sed 's/[][]//g' | sed 's/(//g' | sed 's/)//g' | xargs) + commit_message=$(echo "$commit_message" | sed 's/[<>|()[\]]//g' | xargs)
50-50: Remove unnecessary empty lines in the file pattern section.The empty lines after the file pattern are unnecessary and flagged by the linter.
files: | **.md - -🧰 Tools
🪛 yamllint (1.35.1)
[error] 50-50: trailing spaces
(trailing-spaces)
100-100: Remove trailing whitespace.Remove the trailing whitespace on line 100 to fix the linting error.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 100-100: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/notify-triager.yml(5 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/notify-triager.yml
[error] 50-50: trailing spaces
(trailing-spaces)
[error] 100-100: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/notify-triager.yml (1)
72-72: The reviewer assignment logic looks good.
The conditions correctly handle:
- Code reviewers: Only notified when non-markdown files are changed
- Doc reviewers: Only notified when markdown files are changed
This aligns with the PR objective to fix incorrect notifications.
Let's verify the logic works as expected:
Also applies to: 87-87
✅ Verification successful
Let me try a different approach to verify the logic by examining the workflow file directly.
After examining the workflow file, I can now generate the final response:
The reviewer assignment logic is correctly implemented
The conditions for assigning reviewers are properly structured:
-
Code reviewers are notified when:
- The commit is not a merge commit (
isMergeCommit == 'false') - There are non-markdown files changed (
all_changed_files_count != md-pr-changes.outputs.all_changed_files_count)
- The commit is not a merge commit (
-
Doc reviewers are notified when:
- The commit is not a merge commit (
isMergeCommit == 'false') - Any markdown files are changed (
any_changed == 'true')
- The commit is not a merge commit (
The implementation uses the correct outputs from the changed-files action and properly handles both scenarios, ensuring reviewers are only notified for relevant changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test the reviewer assignment logic with different file change scenarios
# Scenario 1: Only markdown files
echo "Testing scenario with only markdown files..."
gh pr create --draft --title "test: only md changes" --body "test" --base main
touch test.md
git add test.md
git commit -m "test: add markdown file"
git push
# Scenario 2: Mixed files
echo "Testing scenario with mixed files..."
gh pr create --draft --title "test: mixed changes" --body "test" --base main
touch test.md test.js
git add test.md test.js
git commit -m "test: add mixed files"
git push
# Check the workflow runs
gh run list --workflow=notify-triager.yml --limit 2
Length of output: 2339
Script:
#!/bin/bash
# Check the workflow file content
cat .github/workflows/notify-triager.yml
# Find all workflow runs that used this workflow
gh run list --workflow=notify-triager.yml --json conclusion,event,displayTitle --limit 10
Length of output: 5251
.github/workflows/notify-triager.yml
Outdated
| - name: Get all changed files | ||
| id: changed-files | ||
| uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11 | ||
|
|
There was a problem hiding this comment.
Use version tags instead of commit hashes for action versions.
Using a specific commit hash (aa08304bd477b800d468db44fe10f6c61f7f7b11) for the action version is risky as the commit could be force-pushed or deleted. The AI summary indicates that this should be using v45.
- uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11
+ uses: tj-actions/changed-files@v45📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get all changed files | |
| id: changed-files | |
| uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11 | |
| - name: Get all changed files | |
| id: changed-files | |
| uses: tj-actions/changed-files@v45 | |
|
@asyncapi/bounty_team |
.github/workflows/notify-triager.yml
Outdated
| @@ -68,7 +69,7 @@ jobs: | |||
| echo "codeTriagers=$codeTriagers" >> $GITHUB_ENV | |||
|
|
|||
| - name: Add Reviewers for code files | |||
| if: steps.check-merge-branch.outputs.isMergeCommit == 'false' && steps.non-md-pr-changes.outputs.any_changed == 'true' | |||
| if: ${{ steps.check-merge-branch.outputs.isMergeCommit == 'false' && steps.changed-files.outputs.all_changed_files_count != steps.md-pr-changes.outputs.all_changed_files_count }} | |||
There was a problem hiding this comment.
What if the changed-files.outputs.all_changed_files_count is equal to steps.md-pr-changes.outputs.all_changed_files_count?
Can you please explain exactly what that check means?
There was a problem hiding this comment.
For this we are checking for non md files
If the count is not equal that means there are some non md files also which needs to be reviewed by code reviewers
There was a problem hiding this comment.
tj-actions doesn't have any mechanism for non md files so that's why I implemented this or else we need to check for many file extensions
There was a problem hiding this comment.
Though I am still curious about the ambigious behavior of this workflow. For example, examine this PR
See how MD triagers are getting assigned as PR reviewers even though there was no merge commit or a MD file change
Another instance of this issue:
#3075 (comment)
There was a problem hiding this comment.
https://github.com/asyncapi/website/actions/runs/11141478939/job/30962427072
I think this job was surpassed
|
/update |
|
Does this PR need to be reviewed also by @akshatnema before it can be merged? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/notify-triager.yml (2)
19-22: Optimize commit message sanitizationThe commit message sanitization can be simplified and made more robust.
- commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | sed 's/[][]//g' | sed 's/(//g' | sed 's/)//g' | xargs) + commit_message=$(echo "$commit_message" | sed 's/[<>|()[\]]//g' | xargs) + if [ -z "$commit_message" ]; then + echo "Warning: Empty commit message detected" + commit_message="No commit message" + fi
66-66: Remove trailing spacesRemove trailing spaces from lines 66 and 119 for consistency.
Also applies to: 119-119
🧰 Tools
🪛 yamllint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/notify-triager.yml(6 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/notify-triager.yml
42-42: could not parse as YAML: yaml: line 42: could not find expected ':'
(syntax-check)
🪛 yamllint (1.35.1)
.github/workflows/notify-triager.yml
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 43-43: syntax error: could not find expected ':'
(syntax)
[error] 119-119: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
.github/workflows/notify-triager.yml (4)
57-58: Use version tags instead of commit hashes for action versions
Using a specific commit hash for the action version is risky as the commit could be force-pushed or deleted.
Also applies to: 62-63
91-91: LGTM: Improved reviewer assignment logic
The updated conditions correctly handle the assignment of reviewers based on file types:
- Code reviewers are only notified when non-markdown files are changed
- Doc reviewers are only notified when markdown files are changed
This fixes the original issue where code reviewers were incorrectly notified for markdown-only changes.
Also applies to: 106-106
4-4: Verify the implications of changing from pull_request_target to pull_request
The change from pull_request_target to pull_request improves security by running in the context of the fork. However, this might break functionality if the workflow needs access to repository secrets or write permissions.
42-67:
Critical: Resolve merge conflicts in the file counting implementation
The file contains merge conflict markers that must be resolved. There are two competing implementations:
- Custom git diff implementation
- tj-actions/changed-files action
Choose one implementation and remove the other.
If keeping the custom implementation:
- <<<<<<< HEAD
# Initialize counters
md_count=0
non_md_count=0
# Loop through the changed files to count .md and non-.md files
for file in $changed_files; do
if [[ $file == *.md ]]; then
md_count=$((md_count + 1))
else
non_md_count=$((non_md_count + 1))
fi
done
- =======
- # Remove conflicting implementation
- >>>>>>> b0e41ec2418d698e40ee2ef929dd7ccdc0a32227If keeping the tj-actions implementation:
- <<<<<<< HEAD
- # Remove custom implementation
- =======
- name: Get all changed files
id: changed-files
uses: tj-actions/changed-files@v45
- name: Check PR Changes for .md files
id: md-pr-changes
uses: tj-actions/changed-files@v45
with:
files: |
**.md
- >>>>>>> b0e41ec2418d698e40ee2ef929dd7ccdc0a32227Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
42-42: could not parse as YAML: yaml: line 42: could not find expected ':'
(syntax-check)
🪛 yamllint (1.35.1)
[error] 66-66: trailing spaces
(trailing-spaces)
[error] 43-43: syntax error: could not find expected ':'
(syntax)
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
|
@anshgoyalevil @akshatnema @aeworxet I have updated the PR in this we have removed the tj-actions workflow and have written our own workflow with |
anshgoyalevil
removed request for
quetzalliwrites,
VaishnaviNandakumar,
J0SAL,
BhaswatiRoy and
TRohit20
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/notify-triager.yml (2)
14-14: Consider optimizing the git fetch depth.Setting
fetch-depth: 0fetches the complete repository history, which may impact workflow performance. Since we only need to compare changes between the PR's base and head, consider usingfetch-depth: 2instead.- fetch-depth: 0 + fetch-depth: 2
105-105: Remove trailing spaces.Remove trailing whitespace at the end of the file.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 105-105: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/notify-triager.yml(5 hunks)markdown/docs/guides/validate.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- markdown/docs/guides/validate.md
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/notify-triager.yml
18-18: shellcheck reported issue in this script: SC2086:info:5:42: Double quote to prevent globbing and word splitting
(shellcheck)
38-38: shellcheck reported issue in this script: SC2086:info:17:30: Double quote to prevent globbing and word splitting
(shellcheck)
38-38: shellcheck reported issue in this script: SC2086:info:18:38: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint (1.35.1)
.github/workflows/notify-triager.yml
[error] 105-105: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/notify-triager.yml (1)
77-77: LGTM: Reviewer assignment logic matches PR objectives.
The conditions correctly implement the requirement to prevent incorrect notifications:
- Code reviewers are only notified when non-markdown files are changed
- Documentation reviewers are only notified when markdown files are changed
Let's verify the fix works as expected:
Also applies to: 92-92
✅ Verification successful
Let me verify the workflow logic directly by examining the file content and conditions.
Reviewer assignment logic is correctly implemented
The workflow implementation correctly handles the notification logic:
- Code reviewers are notified only when non-markdown files are changed (
steps.changed_files.outputs.non_md_count > 0) - Doc reviewers are notified only when markdown files are changed (
steps.changed_files.outputs.md_count > 0) - Both conditions properly check for non-merge commits (
steps.check-merge-branch.outputs.isMergeCommit == 'false') - The file counting logic in the "Count changed files" step accurately differentiates between markdown and non-markdown files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Test the fix with different file change scenarios
# Note: This is a dry run simulation
echo "Scenario 1: Only markdown files changed"
git diff --name-only "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}" | grep '\.md$' || true
echo "Scenario 2: Only non-markdown files changed"
git diff --name-only "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}" | grep -v '\.md$' || true
Length of output: 494
Script:
#!/bin/bash
# Check the workflow file content
cat .github/workflows/notify-triager.yml
Length of output: 4104
|
/rtm |
The current workflow has an error in which it calls the code reviewers also when the changes are only in the md files
Fixed that by adding a check
fixes : #3214
Summary by CodeRabbit
New Features
Bug Fixes