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.

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

tools: fix `GitHub actions status when CQ is empty #41193

Merged
merged 1 commit into from Dec 18, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Dec 16, 2021

Didn't have time to test it, I'm hoping this would solve the issue (and would also avoid pulling the repo if there are no PRs with commit-queue or request-ci labels).

Refs: #40985 (comment)

@aduh95 aduh95 requested a review from Mesteery December 16, 2021 01:00
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Dec 16, 2021
@Mesteery
Copy link
Member

Mesteery commented Dec 16, 2021

continue-on-error should be removed because gh pr list does not fail if it does not find any pull requests.
And if: steps.get_prs_for_ci.outputs.numbers (the output will be empty if no pull request has been found) should be used instead of if: success().

@targos
Copy link
Member

targos commented Dec 16, 2021

to avoid having to do a lot of if: condition, we could refactor the workflow into two different jobs:

  • first job runs the Get Pull Requests step
  • second job is conditional, depends on the first one and runs the other steps

wdyt?

Edit: bonus would be that the workflow graph would clearly show whether there were PRs to land or not.

@aduh95
Copy link
Contributor Author

aduh95 commented Dec 16, 2021

to avoid having to do a lot of if: condition, we could refactor the workflow into two different jobs:

  • first job runs the Get Pull Requests step
  • second job is conditional, depends on the first one and runs the other steps

wdyt?

I love the idea, unfortunately there are no easy way to share the result between the two jobs (the only solution I found was to upload an artifact, not ideal, overkill for this use case). I've made the if if if solution finally works on my fork, I'm going to push that here, but if someone thinks of something more elegant, I'll take it.

@targos
Copy link
Member

targos commented Dec 16, 2021

there are no easy way to share the result between the two jobs

You can use outputs of a job in another. See https://github.com/zakodium/workflows/blob/main/.github/workflows/release.yml#L26-L54

@aduh95 aduh95 requested review from targos and removed request for Mesteery December 16, 2021 15:01
.github/workflows/commit-queue.yml Outdated Show resolved Hide resolved
.github/workflows/auto-start-ci.yml Outdated Show resolved Hide resolved
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 17, 2021

This could use another review so I can land sooner. @Trott maybe you'd want to do that, since you are the one who reported the issue in #40985 (comment).

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 18, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 18, 2021
@nodejs-github-bot nodejs-github-bot merged commit 587b167 into nodejs:master Dec 18, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 587b167

@aduh95 aduh95 deleted the fix-cq-status branch December 18, 2021 11:14
@Trott
Copy link
Member

Trott commented Dec 18, 2021

Now it looks like request-ci isn't working at all and this job fails even when it finds PRs.... https://github.com/nodejs/node/runs/4569665939?check_suite_focus=true

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 7, 2022

One drawback of the approach in this PR is there's now a race condition on the Auto-Start CI action (and probably the CQ too): #41388

From what I understand, this would happen when there are too many running jobs on the org that it starts to queue up, the get_prs_for_ci jobs run before one of the startCI jobs has time to remove the label. There are no terrible consequences – and hopefully it should not happen too often – but it's a bit annoying. Ideally we would want to skip these actions if the previous one hasn't ran yet, not sure it's actually doable though.

targos pushed a commit that referenced this pull request Jan 14, 2022
Refs: #40985 (comment)

PR-URL: #41193
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
Refs: #40985 (comment)

PR-URL: #41193
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Refs: nodejs#40985 (comment)

PR-URL: nodejs#41193
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
Refs: #40985 (comment)

PR-URL: #41193
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants