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

Improve pull request CI #393

Open
chlowell opened this issue Feb 24, 2023 · 3 comments
Open

Improve pull request CI #393

chlowell opened this issue Feb 24, 2023 · 3 comments

Comments

@chlowell
Copy link
Collaborator

As configured, CI runs only when a PR is labeled or merged to dev or main (see the linting action, for example). This prevents non-maintainers from executing malicious code in CI, which is great. However, it also stops CI from running automatically when a PR is updated with new commits--a maintainer must add or remove a label to trigger CI. There may be ways to improve this with Action configuration. We could also separate the "safe" tests with no access to live resources or secrets and run those on PR updates.

@bgavrilMS
Copy link
Member

As configured, CI runs only when a PR is labeled or merged to dev or main (see the linting action, for example). This prevents non-maintainers from executing malicious code in CI, which is great. However, it also stops CI from running automatically when a PR is updated with new commits--a maintainer must add or remove a label to trigger CI. There may be ways to improve this with Action configuration. We could also separate the "safe" tests with no access to live resources or secrets and run those on PR updates.

It should be sufficient to disable PR builds for PRs originating from forks.

@chlowell
Copy link
Collaborator Author

chlowell commented Jul 6, 2023

I want to run CI on PRs by default though, and run it again on each push, to prevent merging broken code. I assume the motivation for the current setup was to avoid exposing secrets to everyone who opens a PR, however it's unnecessary because GitHub doesn't pass secrets to workflows triggered by PRs from a fork anyway.

There's more work here than reconfiguring the Action triggers though--we also need to skip the integration tests for external PRs because those tests need the secrets and will just fail without them (for example). The simple solution is to skip these tests when the secrets are absent, however that would enable a misconfigured run to succeed without trying all tests; it would be best to make the misconfiguration obvious by failing the run.

@bgavrilMS
Copy link
Member

In my experience, the number of contributors slowly drops as the complexity of the library increases and rarely do contributors write tests anyway. So might as well optimize for your daily flow.

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

No branches or pull requests

2 participants