search

AzureAD / microsoft-authentication-library-for-go

The MSAL library for Go is part of the Microsoft identity platform for developers (formerly named Azure AD) v2.0. It enables you to acquire security tokens to call protected APIs. It uses industry standard OAuth2 and OpenID Connect.
MIT License
233 stars 89 forks source link

Improve pull request CI #393

Open chlowell opened 1 year ago

chlowell commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.