microsoft-authentication-library-for-go icon indicating copy to clipboard operation
microsoft-authentication-library-for-go copied to clipboard

Improve pull request CI

Open chlowell opened this issue 2 years ago • 3 comments

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.

chlowell avatar Feb 24 '23 23:02 chlowell

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.

bgavrilMS avatar Apr 18 '23 17:04 bgavrilMS

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.

chlowell avatar Jul 06 '23 22:07 chlowell

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.

bgavrilMS avatar Jul 07 '23 10:07 bgavrilMS