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: refactor notify triager workflow #3403

Closed
wants to merge 12 commits into from
Closed

fix: refactor notify triager workflow #3403

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9758be5
initial commit
sambhavgupta0705 Nov 17, 2024
3961fef
Merge branch 'master' into triagers
anshgoyalevil Nov 28, 2024
f267006
Merge branch 'master' into triagers
anshgoyalevil Dec 5, 2024
4fa6294
Merge branch 'master' into triagers
sambhavgupta0705 Dec 6, 2024
cb72017
revert
sambhavgupta0705 Dec 6, 2024
6223409
fix
sambhavgupta0705 Dec 6, 2024
c44dbec
Merge branch 'triagers' of github.com:sambhavgupta0705/asyncapi_websi…
sambhavgupta0705 Dec 6, 2024
b0bbf6c
fixed the spacing
sambhavgupta0705 Dec 9, 2024
097ad18
testing
sambhavgupta0705 Dec 9, 2024
541568c
change of codeowners
sambhavgupta0705 Dec 9, 2024
55a0c57
test
sambhavgupta0705 Dec 9, 2024
5268cfb
test
sambhavgupta0705 Dec 9, 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
58 changes: 20 additions & 38 deletions .github/workflows/notify-triager.yml
View file
Open in desktop
Copy link
Member

@anshgoyalevil anshgoyalevil Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the previous implementation of notify traiger workflow, we were getting doc triagers assigned to the PRs that didn't contain any doc changes. This was because of how the workflow was checking for file changes in the PR.

For example, in previous implementation:

  • User A created a PR P1 with code changes. -> Only code triagers assigned to the PR
  • User B created a PR P2 with doc changes. -> Only doc triagers assigned to the PR
  • PR P2 gets merged first.
  • PR P1 is updated to include a merge commit containing PR P2 changes -> Doc traigers also gets assigned to this PR P1. Now this is the part where we are going wrong. Also, this behavior is still ambigious

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was because of MD file changes

Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,26 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout Repository
uses: actions/checkout@v4.1.1
uses: actions/checkout@v4
with:
repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.event.pull_request.head.sha }}

- name: Get commit message
id: commit-message
run: |
commit_message=$(git log --format=%B -n 1 ${{ github.event.pull_request.head.sha }})
echo "commit_message=$commit_message" >> $GITHUB_OUTPUT
commit_message=$(git log --format=%B -n 1 "${{ github.event.pull_request.head.sha }}")
echo "commit_message=${commit_message}" >> $GITHUB_ENV

- name: Check if last commit is a merge commit
id: check-merge-branch
run: |
if [[ "${{ steps.commit-message.outputs.commit_message }}" == *"Merge branch"* ]]; then
echo "Last commit is a merge commit"
echo "isMergeCommit=true" >> $GITHUB_OUTPUT
if [[ "${{ env.commit_message }}" == *"Merge branch"* ]]; then
echo "isMergeCommit=true" >> $GITHUB_ENV
else
echo "Last commit message does not contain Merge branch"
echo "isMergeCommit=false" >> $GITHUB_OUTPUT
echo "isMergeCommit=false" >> $GITHUB_ENV
fi
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve shell script robustness and merge detection

The shell script has several areas for improvement:

  1. Missing quotes around variables (flagged by shellcheck)
  2. Merge detection could miss other merge patterns

Apply these improvements:

      - name: Get commit message
        id: commit-message
        run: |
-          commit_message=$(git log --format=%B -n 1 "${{ github.event.pull_request.head.sha }}")
-          echo "commit_message=${commit_message}" >> $GITHUB_ENV
+          commit_message="$(git log --format=%B -n 1 "${{ github.event.pull_request.head.sha }}")"
+          echo "commit_message=${commit_message}" >> "$GITHUB_ENV"

      - name: Check if last commit is a merge commit
        id: check-merge-branch
        run: |
-          if [[ "${{ env.commit_message }}" == *"Merge branch"* ]]; then
-            echo "isMergeCommit=true" >> $GITHUB_ENV
+          if git rev-parse --verify "${{ github.event.pull_request.head.sha }}^2" >/dev/null 2>&1; then
+            echo "isMergeCommit=true" >> "$GITHUB_ENV"
           else
-            echo "isMergeCommit=false" >> $GITHUB_ENV
+            echo "isMergeCommit=false" >> "$GITHUB_ENV"
           fi
📝 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
run: |
commit_message=$(git log --format=%B -n 1 ${{ github.event.pull_request.head.sha }})
echo "commit_message=$commit_message" >> $GITHUB_OUTPUT
commit_message=$(git log --format=%B -n 1 "${{ github.event.pull_request.head.sha }}")
echo "commit_message=${commit_message}" >> $GITHUB_ENV
- name: Check if last commit is a merge commit
id: check-merge-branch
run: |
if [[ "${{ steps.commit-message.outputs.commit_message }}" == *"Merge branch"* ]]; then
echo "Last commit is a merge commit"
echo "isMergeCommit=true" >> $GITHUB_OUTPUT
if [[ "${{ env.commit_message }}" == *"Merge branch"* ]]; then
echo "isMergeCommit=true" >> $GITHUB_ENV
else
echo "Last commit message does not contain Merge branch"
echo "isMergeCommit=false" >> $GITHUB_OUTPUT
echo "isMergeCommit=false" >> $GITHUB_ENV
fi
run: |
commit_message="$(git log --format=%B -n 1 "${{ github.event.pull_request.head.sha }}")"
echo "commit_message=${commit_message}" >> "$GITHUB_ENV"
- name: Check if last commit is a merge commit
id: check-merge-branch
run: |
if git rev-parse --verify "${{ github.event.pull_request.head.sha }}^2" >/dev/null 2>&1; then
echo "isMergeCommit=true" >> "$GITHUB_ENV"
else
echo "isMergeCommit=false" >> "$GITHUB_ENV"
fi
🧰 Tools
🪛 actionlint (1.7.4)

19-19: shellcheck reported issue in this script: SC2086:info:2:44: Double quote to prevent globbing and word splitting

(shellcheck)

25-25: shellcheck reported issue in this script: SC2193:warning:1:35: The arguments to this comparison can never be equal. Make sure your syntax is correct

(shellcheck)

25-25: shellcheck reported issue in this script: SC2086:info:2:32: Double quote to prevent globbing and word splitting

(shellcheck)

25-25: shellcheck reported issue in this script: SC2086:info:4:33: Double quote to prevent globbing and word splitting

(shellcheck)


- name: Checkout asyncapi/website Repository
uses: actions/[email protected]

- 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
Expand All @@ -48,51 +43,38 @@ jobs:
files: |
!**.md


- name: Extract Doc Triage Maintainers
id: doc-triager
run: |
docTriagers=$(grep '^#' CODEOWNERS | tail -n 2 | head -n 1)
echo "docTriagers: $docTriagers"
prefix="#docTriagers: "
docTriagers=${docTriagers#$prefix}
docTriagers=$(awk '/^#docTriagers: / {print substr($0, index($0,$2))}' CODEOWNERS)
echo "docTriagers=$docTriagers" >> $GITHUB_ENV

- name: Extract Code Triage Maintainers
id: code-triager
run: |
codeTriagers=$(grep '^#' CODEOWNERS | tail -n 1)
echo "codeTriagers: $codeTriagers"
prefix="#codeTriagers: "
codeTriagers=${codeTriagers#$prefix}
codeTriagers=$(awk '/^#codeTriagers: / {print substr($0, index($0,$2))}' CODEOWNERS)
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: env.isMergeCommit == 'false' && steps.non-md-pr-changes.outputs.any_changed == 'true'
run: |
IFS=' ' read -r -a codeTriagers <<< "${{ env.codeTriagers }}"
reviewers=$(printf ', "%s"' "${codeTriagers[@]}")
reviewers=[${reviewers:2}]
curl \
-X POST \
-H "Authorization: token ${{ secrets.GH_TOKEN }}" \
-H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers \
-d "{
\"reviewers\": $reviewers
}"
curl -X POST \
-H "Authorization: token ${{ secrets.GH_TOKEN }}" \
-H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers \
-d "{\"reviewers\": $reviewers}"

- name: Add Reviewers for doc files
if: steps.check-merge-branch.outputs.isMergeCommit == 'false' && steps.md-pr-changes.outputs.any_changed == 'true'
if: env.isMergeCommit == 'false' && steps.md-pr-changes.outputs.any_changed == 'true'
run: |
IFS=' ' read -r -a docTriagers <<< "${{ env.docTriagers }}"
reviewers=$(printf ', "%s"' "${docTriagers[@]}")
reviewers=[${reviewers:2}]
curl \
-X POST \
-H "Authorization: token ${{ secrets.GH_TOKEN }}" \
-H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers \
-d "{
\"reviewers\": $reviewers
}"
curl -X POST \
-H "Authorization: token ${{ secrets.GH_TOKEN }}" \
-H "Accept: application/vnd.github.v3+json" \
https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/requested_reviewers \
-d "{\"reviewers\": $reviewers}"
Loading