Skip to content
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.

Sign up for GitHub

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

fix: fixed the notify triager workflow #3460

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+11 −9
Open

fix: fixed the notify triager workflow #3460

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76fd8e4
fix: fixed the notify triager workflow
sambhavgupta0705 Dec 9, 2024
e230dde
Merge branch 'master' of github.com:sambhavgupta0705/asyncapi_website…
sambhavgupta0705 Dec 9, 2024
02b18ed
Update notify-triager.yml
sambhavgupta0705 Dec 9, 2024
aadb343
Merge branch 'master' into bounty2
sambhavgupta0705 Dec 10, 2024
abdc1f1
Merge branch 'master' into bounty2
sambhavgupta0705 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions .github/workflows/notify-triager.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ jobs:
- name: Get commit message
id: commit-message
run: |
# Extract the commit message
commit_message=$(git log --format=%B -n 1 ${{ github.event.pull_request.head.sha }})
commit_message=$(echo "$commit_message" | tr '\n' ' ')
commit_message=$(echo "$commit_message" | sed 's/[<>|]//g' | sed 's/[][]//g' | sed 's/(//g' | sed 's/)//g' | xargs)
echo "commit_message=$commit_message" >> $GITHUB_OUTPUT

- name: Check if last commit is a merge commit
Expand All @@ -34,19 +37,17 @@ jobs:
- name: Checkout asyncapi/website Repository
uses: actions/[email protected]

- name: Get all changed files
id: changed-files
uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11

Comment on lines +40 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
- 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

- name: Check PR Changes for .md files
id: md-pr-changes
uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11 # version 42.1.0 https://github.com/tj-actions/changed-files/releases/tag/v42.1.0
with:
files: |
**.md

- name: Check PR Changes for non-.md files
id: non-md-pr-changes
uses: tj-actions/changed-files@aa08304bd477b800d468db44fe10f6c61f7f7b11 # version 42.1.0 https://github.com/tj-actions/changed-files/releases/tag/v42.1.0
with:
files: |
!**.md



- name: Extract Doc Triage Maintainers
Expand All @@ -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 }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Good catch

Copy link
Member

@anshgoyalevil anshgoyalevil Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I am still curious about the ambigious behavior of this workflow. For example, examine this PR

#3245 (comment)

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/asyncapi/website/actions/runs/11141478939/job/30962427072

I think this job was surpassed

run: |
IFS=' ' read -r -a codeTriagers <<< "${{ env.codeTriagers }}"
reviewers=$(printf ', "%s"' "${codeTriagers[@]}")
Expand All @@ -83,7 +84,7 @@ jobs:
}"

- name: Add Reviewers for doc files
if: steps.check-merge-branch.outputs.isMergeCommit == 'false' && steps.md-pr-changes.outputs.any_changed == 'true'
if: ${{ steps.check-merge-branch.outputs.isMergeCommit == 'false' && steps.md-pr-changes.outputs.any_changed == 'true' }}
run: |
IFS=' ' read -r -a docTriagers <<< "${{ env.docTriagers }}"
reviewers=$(printf ', "%s"' "${docTriagers[@]}")
Expand All @@ -96,3 +97,4 @@ jobs:
-d "{
\"reviewers\": $reviewers
}"

Loading