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

Remove approval block to run tests, lint, and annotations #133

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zzak
Copy link
Member

@zzak zzak commented Dec 2, 2024

These steps are designed so any code executed from the workspace happens inside an unprivileged docker container.

We would get faster feedback on PRs if these run instantly.

@matthewd
Copy link
Member

matthewd commented Dec 2, 2024

Moving it is fine (I'd wondered why the lint checks were placed before it), but removing "this is an important security gate" with "avoid spending extra build resources" [do we really have enough PRs here for that to be important?] is clearly not okay.

These steps are designed so any code executed from the workspace happens
inside an unprivileged docker container.

We would get faster feedback on PRs if these run instantly.
@zzak zzak force-pushed the lint-test-no-review branch from 1d56bf7 to bd7fd1a Compare December 2, 2024 21:17
@zzak
Copy link
Member Author

zzak commented Dec 2, 2024

is clearly not okay

You're 100% right, I was wondering how to update the messaging but meant to ask that in review. Sorry for not clarifying. How does bd7fd1a look?

avoid spending extra build resources

My original thought was not cost for BK, but that we had (have?) a max 100 agents available and one build can take a significant amount of them, so triggering a bunch of test CI runs will quickly result in many pending jobs. That is less common, given how few PRs we get but wanted to call that out in the review.

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

Successfully merging this pull request may close these issues.

2 participants