-
Notifications
You must be signed in to change notification settings - Fork 64
Ensure skip-ci label workable and simplify profiling job conditions #877
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
6de91a6
270b7c1
6664c81
4aa1ee7
d63f21f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,50 +54,56 @@ jobs: | |
|
|
||
| - name: scan label, description, and comments | ||
| id: check | ||
| if: github.event_name == 'pull_request' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const trusted = ['OWNER', 'MEMBER', 'COLLABORATOR']; | ||
| // The control string is honored only when it is alone on a line | ||
| // by itself, so that prose mentioning it does not accidentally | ||
| // skip CI. | ||
| const control = /^\[skip-ci\]$/i; | ||
| const hasControlLine = text => | ||
| (text || '').split(/\r?\n/) | ||
| .some(line => control.test(line.trim())); | ||
| const pr = context.payload.pull_request; | ||
|
|
||
| // 1. The skip-ci label, set by a repository member. | ||
| if (pr.labels.some(label => label.name === 'skip-ci')) { | ||
| core.notice('CI skipped: the "skip-ci" label is present.'); | ||
| core.setOutput('skip', 'true'); | ||
| return; | ||
| async function hasSkipLabelOnPr(prNumber) { | ||
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: prNumber, | ||
| }); | ||
|
|
||
| if (pr.labels && pr.labels.some(label => label.name === 'skip-ci')) { | ||
| core.notice(`CI skipped: the "skip-ci" label is present on PR #${pr.number}.`); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // 2. The control string in the pull request description. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I cannot find the checking for the control string. Please point out where it is in the new code. |
||
| if (trusted.includes(pr.author_association) && | ||
| hasControlLine(pr.body)) { | ||
| core.notice('CI skipped: control string in the description.'); | ||
| core.setOutput('skip', 'true'); | ||
| if (github.event_name === 'pull_request') { | ||
| const pr = context.payload.pull_request; | ||
| if (!pr) { | ||
| core.setOutput('skip', 'false'); | ||
| return; | ||
| } | ||
| const shouldSkip = await hasSkipLabelOnPr(pr.number); | ||
| core.setOutput('skip', shouldSkip ? 'true' : 'false'); | ||
| return; | ||
| } | ||
|
|
||
| // 3. The control string in a comment from a member. | ||
| const comments = await github.paginate( | ||
| github.rest.issues.listComments, | ||
| { | ||
| else if (github.event_name === 'push') { | ||
| const sha = context.sha || context.payload.after; | ||
| if (!sha) { | ||
| core.setOutput('skip', 'false'); | ||
| return; | ||
| } | ||
|
|
||
| const assoc = await github.rest.repos.listPullRequestsAssociatedWithCommit({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: pr.number, | ||
| per_page: 100, | ||
| commit_sha: sha, | ||
| }); | ||
| const hit = comments.find( | ||
| comment => trusted.includes(comment.author_association) && | ||
| hasControlLine(comment.body)); | ||
| if (hit) { | ||
| core.notice('CI skipped: control string in a comment.'); | ||
| core.setOutput('skip', 'true'); | ||
|
|
||
| const prs = assoc.data || []; | ||
| for (const p of prs) { | ||
| if (await hasSkipLabelOnPr(p.number)) { | ||
| core.setOutput('skip', 'true'); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| core.setOutput('skip', 'false'); | ||
| return; | ||
| } | ||
|
Comment on lines
+74
to
108
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,19 @@ | ||
| name: profiling | ||
|
|
||
| on: | ||
| push: | ||
| pull_request: | ||
|
Comment on lines
-4
to
-5
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed these 2 triggers |
||
| schedule: | ||
| - cron: '34 17 * * *' | ||
| workflow_dispatch: | ||
| push: | ||
| pull_request: | ||
|
Comment on lines
+6
to
+8
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bring
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it a requirement to move them after |
||
|
|
||
| jobs: | ||
| profile: | ||
|
|
||
| # Run profiling only on schedule or when MMGH_FORCE_PROFILE is set to 'enable' | ||
| # You can refer to https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/use-variables to set MMGH_FORCE_PROFILE in the fork repository settings for testing purposes. | ||
| if: ${{ (github.event_name == 'schedule' && vars.MMGH_NIGHTLY == 'enable') || vars.MMGH_FORCE_PROFILE == 'enable' }} | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So no need to check
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If removing |
||
| # Run profiling on schedule or when MMGH_FORCE_PROFILE is set to 'enable'. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new URL redirects to the original URL. It is not clear to me why the comments need to be updated. What is the reason? If it does not need to, revert the change. |
||
| # MMGH_FORCE_PROFILE can force profiling for push and pull_request events. | ||
| # See: https://docs.github.com/en/actions/learn-github-actions/variables#organization-and-repository-variables | ||
| if: ${{ vars.MMGH_NIGHTLY == 'enable' || vars.MMGH_FORCE_PROFILE == 'enable' }} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not clear to me why dropping |
||
|
|
||
| name: profile_${{ matrix.os }} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helper function is simplified, and focus on
skip-cilabel is existedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asyncmakes it more complex, not simpler. Do you really need async?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
actions/github-script@v7use JavaScript as the script language, we have to useasyncandawaitto make sure the value had been fetched.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JavaScript code before this PR did not use async. Why do we need it now?