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

Add verify-pr-builder hook #57

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

KyleFromNVIDIA
Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA commented Aug 28, 2024

Add hook to verify that the pr-builder job is set up correctly.

This supersedes the rapids-check-pr-job-dependencies tool. The tool only does static analysis and does not depend on the GitHub environment. Such a check can be done on the developer's machine, while also ensuring that the lists are sorted properly, and automatically fixing any mistakes.

Add hook to verify that the pr-builder job is set up correctly.
@KyleFromNVIDIA KyleFromNVIDIA requested review from a team as code owners August 28, 2024 17:47
@jameslamb
Copy link
Member

For your consideration.... the rapidsai/shared-workflows checks and gha-tools scripts are used by other repos outside of RAPIDS.

(GitHub Search)

Some examples:

If we pair a change like this with dropping the corresponding code that currently comes for free with just using shared-workflows (shared-workflows code link), then at a minimum I think we should roll out this hook to those other repos.

I'm weakly against this proposal.

benefits:

  • slightly less wasted CI and developer time, via finding out locally that some GitHub Actions config changes are needed (instead of only finding out after a push + CI run)
  • simplifies development in gha-tools repo
  • simplifies the checks workflow from shared-workflows slightly

costs:

  • elevated risk of silently missing pr-builder dependencies in new repos (as a result of now needing to opt in to this validation via a pre-commit hook)
  • development work of rolling out the hook to all repos currently using rapidsai/shared-workflows
  • increased complexity and maintenance burden here in pre-commit-hooks

Based on those, I think the costs outweigh the benefits and this should be left alone. But I don't feel so strongly enough about this to block this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants