Skip to content

Commit 5afea02

Browse files
authored
fix(workflows): test ci with pull_request_target (aws#5508)
## Problem Just logging the changelog/test notification isn't really a good way ## Solution Revert aws#5506 in favour of trying out pull_request_target. pull_request_target gives us these extra permissions to create these workflows that comment on PR's. I've also moved notification out of the lint so its non-critical path Example run: jpinkney-aws#269 --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent e836e51 commit 5afea02

File tree

4 files changed

+70
-11
lines changed

4 files changed

+70
-11
lines changed

.github/workflows/node.js.yml

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,6 @@ jobs:
3232
- uses: actions/setup-node@v4
3333
with:
3434
node-version: '20'
35-
- name: Check for tests
36-
uses: actions/github-script@v7
37-
with:
38-
script: |
39-
const notify = require('.github/workflows/notify.js')
40-
await notify({github, context})
41-
if: github.event_name == 'pull_request'
4235
- name: Check PR title
4336
run: |
4437
node "$GITHUB_WORKSPACE/.github/workflows/lintcommit.js"

.github/workflows/notification.yml

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
# github actions: https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-nodejs
2+
# setup-node: https://github.com/actions/setup-node
3+
4+
name: Notifications
5+
6+
on:
7+
pull_request_target:
8+
branches: [master, feature/*, staging]
9+
# Default = opened + synchronize + reopened.
10+
# We also want "edited" so that changelog notifications runs when PR title is updated.
11+
# https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request
12+
types:
13+
- edited
14+
- opened
15+
- reopened
16+
- synchronize
17+
18+
# Cancel old jobs when a pull request is updated.
19+
concurrency:
20+
group: ${{ github.workflow }}-${{ github.ref }}
21+
cancel-in-progress: true
22+
23+
jobs:
24+
notify:
25+
runs-on: ubuntu-latest
26+
permissions:
27+
pull-requests: write
28+
issues: read
29+
steps:
30+
- uses: actions/checkout@v4
31+
with:
32+
fetch-depth: 20
33+
- uses: actions/setup-node@v4
34+
with:
35+
node-version: '20'
36+
- name: Check for tests
37+
uses: actions/github-script@v7
38+
with:
39+
script: |
40+
const notify = require('.github/workflows/notify.js')
41+
await notify({github, context})
42+
if: github.event_name == 'pull_request_target'

.github/workflows/notify.js

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
const { parsePRTitle, hasPath } = require('./utils')
6+
const { parsePRTitle, hasPath, dedupComment } = require('./utils')
77

88
const testFilesMessage =
99
'This pull request modifies files in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.'
@@ -21,6 +21,7 @@ module.exports = async ({ github, context }) => {
2121
const owner = context.repo.owner
2222
const repo = context.repo.repo
2323
const author = context.payload.pull_request.head.repo.owner.login
24+
const pullRequestId = context.payload.pull_request.number
2425

2526
const response = await github.rest.repos.compareCommitsWithBasehead({
2627
owner,
@@ -37,13 +38,19 @@ module.exports = async ({ github, context }) => {
3738
return
3839
}
3940

41+
// Check for prior comments on the PR
42+
const comments = await github.rest.issues.listComments({
43+
owner,
44+
repo,
45+
issue_number: pullRequestId,
46+
})
47+
4048
if (shouldAddTestFileMessage) {
41-
// We can't really block on this one, since its valid to make a change in src/ without adding tests :( console.error(testFilesMessage)
49+
await dedupComment({ github, comments, owner, repo, pullRequestId, message: testFilesMessage })
4250
}
4351

4452
if (shouldAddChangelogMessage) {
45-
console.error(changelogMessage)
46-
process.exit(1)
53+
await dedupComment({ github, comments, owner, repo, pullRequestId, message: changelogMessage })
4754
}
4855
}
4956

.github/workflows/utils.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,22 @@ function parsePRTitle(title) {
2525
}
2626
}
2727

28+
/**
29+
* Create a comment on a PR if one does not already exist
30+
*/
31+
async function dedupComment({ github, pullRequestId, owner, repo, comments, message }) {
32+
if (comments.data.some((comment) => comment.body.includes(message))) {
33+
return
34+
}
35+
36+
await github.rest.issues.createComment({
37+
issue_number: pullRequestId,
38+
owner,
39+
repo,
40+
body: message,
41+
})
42+
}
43+
2844
/*
2945
* Check if path is included in at least one of the filename paths
3046
*/
@@ -34,5 +50,6 @@ function hasPath(filenames, path) {
3450

3551
module.exports = {
3652
parsePRTitle,
53+
dedupComment,
3754
hasPath,
3855
}

0 commit comments

Comments
 (0)