Skip to content

Conversation

cpcloud
Copy link
Contributor

@cpcloud cpcloud commented Sep 22, 2025

This PR changes our usage of status-check.yml to allow skipping CI when the head commit of PR contains the text
[skip ci].

Our current usage of the wait-for-check action doesn't sense to me.

We have as inputs all the checks that we care about build, test*, docs,
and we only invoke that action when those succeed.

That seems to be no different than running exit 0 against ubuntu-latest and
thus defeats the purpose of using wait-for-checks which is to allow
finer-grained control over what is actually a required check.

In our case, there are cases such as #999 where it doesn't make sense to do
a full build test docs run, but developers are left waiting on an admin to
merge their PR.

Proposal for merging this PR:

  1. Make the previously not required checks actually required in the GitHub UI.
    I don't have permissions to do that, so someone other than me needs to do
    that.
  2. Merge this PR.
  3. Try out a couple no-op PRs to verify that everything is working.

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Sep 22, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cpcloud cpcloud force-pushed the avoid-status-checks-with-skip-ci branch from b455cfe to 7a583d3 Compare September 23, 2025 20:12
@cpcloud cpcloud force-pushed the avoid-status-checks-with-skip-ci branch from 7a583d3 to 6b6844a Compare September 30, 2025 16:36
@cpcloud cpcloud requested review from kkraus14 and leofang September 30, 2025 16:36
@leofang
Copy link
Member

leofang commented Oct 1, 2025

/ok to test 6563550

@github-actions

This comment has been minimized.

@cpcloud cpcloud force-pushed the avoid-status-checks-with-skip-ci branch from 6563550 to d1ec467 Compare October 1, 2025 14:09
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud cpcloud marked this pull request as ready for review October 1, 2025 14:23
@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Oct 1, 2025

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@leofang
Copy link
Member

leofang commented Oct 1, 2025

Check job still completes instead of spinning: https://github.com/NVIDIA/cuda-python/actions/runs/18165463144/job/51706593048?pr=1008

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

@leofang Can you paste the names of the checks here from the UI? I can't see them because I don't have access.

@leofang
Copy link
Member

leofang commented Oct 1, 2025

Once we are sure the check job completes the last, we can either adjust the repo setting and then merge, or admin-merge then adjust the repo setting. But we are not there yet. The repo setting is irrelevant of the failure of waiting for dependent jobs here. It is just a workflow bug that we should fix first.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

1 similar comment
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

I'm going to try one more alternative to avoid the action entirely.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

I really don't like having to add an arbitrary delay to get this to work.

@leofang
Copy link
Member

leofang commented Oct 1, 2025

Yeah adding a random delay is flakey. It could proceed to complete when build jobs finish but the test jobs are still being spawned. Not a fan either.

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

3 similar comments
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud cpcloud changed the title ci: replace single gating check with wait-for-status-check ci: replace single gating check with wait-for-status-check [skip ci] Oct 1, 2025
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@leofang
Copy link
Member

leofang commented Oct 1, 2025

The current Check job status / Check job status required check needs to be changed to CI / Check job status.

I applied it. You can see it now becomes "required". I haven't removed the old check yet.

But I am confused: If this is required, we still need to execute /ok to test, skip build/test/doc jobs, and make this job execute and turn green, right? But the current logic (mysteriously) also skips the status check job. Am I missing something?

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks

The new status check job should be considered passing, because it's skipped, according to that documentation.

I'm try to taking advantage of the fact that skip == success.

@cpcloud cpcloud changed the title ci: replace single gating check with wait-for-status-check [skip ci] ci: replace single gating check with wait-for-status-check [no-ci] Oct 1, 2025
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud cpcloud force-pushed the avoid-status-checks-with-skip-ci branch from 8203214 to 1685f32 Compare October 1, 2025 15:58
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

/ok to test

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

@leofang It looks like the push check is somehow different from the check that's waiting. Are push checks separate in the checks UI?

@leofang
Copy link
Member

leofang commented Oct 1, 2025

I am checking. This is odd because as discussed earlier once a CI job is run, it should appear as a registered job in the repo setting's dropdown menu, but for some reason I had to type the full name explicitly without anything popping out for me to choose.

@leofang
Copy link
Member

leofang commented Oct 1, 2025

This is what's in the ruleset that Keith set up. I added the last row manually. Nothing is "push"-specific 🤔
截圖 2025-10-01 下午12 04 57

@leofang
Copy link
Member

leofang commented Oct 1, 2025

@cpcloud I think I fixed it. It's just "Check job status" (without "CI"), see the last row:
截圖 2025-10-01 下午12 11 17
You can see your new job becomes required now. But, it does not seem to be allowing a green merge button? Is it because I still haven't removed the old check?

@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

Is it because I still haven't removed the old check?

Yep, that has to be removed before merge is allowed.

@leofang
Copy link
Member

leofang commented Oct 1, 2025

Let me check quickly. btw we'll need to backport this PR to 12.9.x. Keith applied the same ruleset to all branches.

@leofang leofang merged commit 598874c into NVIDIA:main Oct 1, 2025
13 checks passed
@cpcloud cpcloud deleted the avoid-status-checks-with-skip-ci branch October 1, 2025 16:17
@leofang
Copy link
Member

leofang commented Oct 1, 2025

It works now! To avoid race condition I am merging immediately.

@leofang leofang added CI/CD CI/CD infrastructure P1 Medium priority - Should do labels Oct 1, 2025
@leofang leofang added this to the cuda.core beta 7 milestone Oct 1, 2025
@cpcloud
Copy link
Contributor Author

cpcloud commented Oct 1, 2025

Thanks for the review, much appreciated!

@leofang
Copy link
Member

leofang commented Oct 1, 2025

Thanks for working this out Phillip!

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

Doc Preview CI
Preview removed because the pull request was closed or merged.

cpcloud added a commit that referenced this pull request Oct 1, 2025
…#1056)

* ci: replace single gating check with vanilla skipping (#1008)

* ci: remove docs

* ci: add back matrix filter
@cpcloud cpcloud mentioned this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD infrastructure P1 Medium priority - Should do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants