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

Update lint.yml #177

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Update lint.yml #177

merged 1 commit into from
Jul 25, 2023

Conversation

akotlar
Copy link
Collaborator

@akotlar akotlar commented Jul 25, 2023

  • Updates GitHub actions to run on pull_request in addition to push.

We may want to limit to only running tests on pull request, because GitHub actions cost us money and will have a lot of noise (every commit push to the repo), but leaving for now until we know that we don't need them.

@akotlar akotlar requested a review from poneill July 25, 2023 10:51
@akotlar akotlar mentioned this pull request Jul 25, 2023
Copy link
Contributor

@poneill poneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there's a question of whether we want pull_request or pull_request_target here, the issue being that allowing 3rd party contributors to submit PRs with CI checks that run according to infra code in their own branch, rather than the infra code in their target branch, is something of a security risk. (see: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/).

But this is an improvement over the status quo, so approving for now.

If we are concerned about costs I'd rather configure CI so that it doesn't run on every push, but still keep the full pipeline in place and require passing checks on the last commit before merging. Ruff and mypy will still catch things that a less-than-perfect test suite won't.

@poneill poneill merged commit 3b3c7ef into master Jul 25, 2023
4 checks passed
@akotlar akotlar deleted the akotlar-patch-1 branch October 17, 2023 04:37
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