Conversation
Micky774
requested review from
ipanfilo,
wangye805 and
wenchenvincent
as code owners
ipanfilo
left a comment
ipanfilo
left a comment
There was a problem hiding this comment.
Maybe make approach more generic - ci_level% label specifies CI level to use.
Micky774
changed the title
| let test_level = requires_dispatch ? level : ''; | ||
| core.setOutput('test_level', test_level); | ||
| core.setOutput('current_level', String(currentLevel)); | ||
| const shouldCancelOthers = (test_level !== '') || (action === 'unlabeled' && removedWasHighest); |
There was a problem hiding this comment.
Removing label should not automatically cancel running CI
There was a problem hiding this comment.
Now only removing the last remaining ci-level label will cancel past CI. I want this to serve as an explicit request to save resources on unnecessary tests.
| // Determine if a CI level label was added, and what level it was. | ||
| const addedLevel = action === 'labeled' ? parseLevelLabel(context.payload.label?.name) : 0; | ||
|
|
||
| core.info(`Dispatch debug: action=${action}, addedLabel=${context.payload.label?.name || ''}, addedLevel=${addedLevel}, currentLevel=${currentLevel}, labels=[${labels.join(',')}]`); |
There was a problem hiding this comment.
Info here makes sense only for labeled action
| const sameSha = run.head_sha === headSha; | ||
| const sameHeadRef = run.head_branch === context.payload.pull_request.head.ref; | ||
| const activeOrDone = | ||
| run.status === 'queued' || |
There was a problem hiding this comment.
This looks like all possible status values
There was a problem hiding this comment.
I misunderstood the API a bit -- I've updated to what I think is a more accurate reflection of what I want (basically it can be any of the active statuses, OR it can be completed AND successful).
|
|
||
| const candidateRuns = runs.filter(run => { | ||
| const prMatch = (run.pull_requests || []).some(pr => pr.number === prNumber); | ||
| const sameSha = run.head_sha === headSha; |
There was a problem hiding this comment.
head_sha is possible query parameter for listWorkflowRuns
| core.info(`Dispatch debug: total workflow runs fetched=${runs.length}`); | ||
|
|
||
| const candidateRuns = runs.filter(run => { | ||
| const prMatch = (run.pull_requests || []).some(pr => pr.number === prNumber); |
There was a problem hiding this comment.
According to documentation this list contains all PR's with matching head_sha or head_branch so it does not help to associate the PR. IMHO the most precise matching is branch query parameter to match the PR target branch and head_sha and head_branch for the run both match the PR too.
Even this one is not ideal because PR run does merging of source and target so strictly speaking it may run different code after each target update
There was a problem hiding this comment.
Could you help me understand what scenario might this fail in specifically? I've updated it to only rely on the PR match + head sha. Is there a situation where that wouldn't be precise enough?
There was a problem hiding this comment.
One scenario is base branch change for PR. And the other is target branch update - they do not directly affect PR code, so head_sha remains the same but test results may differ.
It is OK not to automatically rerun CI in those cases though because they are rather corner cases and mainstream usage will not include label playing w/o code touching.
By the same reason it is not to worry mach about multiple PRs from the same commit to different targets.
| if (m) { | ||
| detectedLevel = Math.max(detectedLevel, Number(m[1])); | ||
| } | ||
| core.info(`Dispatch debug: run_id=${run.id}, job_id=${job.id}, job_name=${job.name}, detectedLevel=${detectedLevel}`); |
There was a problem hiding this comment.
Potentially excessive logging with this and the next message in the loop
| core.setOutput('test_level', requiresDispatch ? String(currentLevel) : ''); | ||
|
|
||
| maybe_cancel_all: | ||
| if: ${{ github.event.action == 'unlabeled' }} |
There was a problem hiding this comment.
I'm still opposed of using labels as a tool to stop workflowd. User may cancel workflow in actions menu, if they want
There was a problem hiding this comment.
I've removed the cancellation now.
| core.info(`Dispatch debug: total workflow runs fetched=${runs.length}`); | ||
|
|
||
| const candidateRuns = runs.filter(run => { | ||
| const prMatch = (run.pull_requests || []).some(pr => pr.number === prNumber); |
There was a problem hiding this comment.
One scenario is base branch change for PR. And the other is target branch update - they do not directly affect PR code, so head_sha remains the same but test results may differ.
It is OK not to automatically rerun CI in those cases though because they are rather corner cases and mainstream usage will not include label playing w/o code touching.
By the same reason it is not to worry mach about multiple PRs from the same commit to different targets.
| core.info(`Dispatch debug: candidate runs matching PR+sha=${candidateRuns.length}`); | ||
|
|
||
| let satisfiesAddedLevel = false; | ||
| for (const run of candidateRuns) { |
There was a problem hiding this comment.
Doesn't run-name (CI Level) appear in github.rest.actions.listWorkflowRuns?
2 participants
Description
With this PR, testing is mediated by labels such as:
ci-level {1, 2, 3}. No tests will be run without at least one label present. At any given time, the highest-level label will take precedence, and only its corresponding level of tests will be run. The workflow triggers on labeling, unlabeling, and pushing commits. When removing a label, the workflow will check to see if there are any labels remaining and if there are, it will dispatch to the highest of them.Fixes # (issue)
Type of change
Changes
Please list the changes introduced in this PR:
Checklist: