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.

Sign up for GitHub

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

Jump to bottom

prevent poisoning via build job #961

Merged
merged 4 commits into from
Nov 3, 2022
Merged

prevent poisoning via build job #961

merged 4 commits into from
Nov 3, 2022

Conversation

vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Oct 28, 2022 •
edited
Loading

Close #959 (for realz?)

  • building only downloads the deps needed by non-test code, which is a smaller subset. We fix it by calling go mod download before go build, which will get all dependencies
  • Protobuf build also poisoned the cache, disabled cache there since its a fast build
  • WASM also poisoned the cache, disabled cache there since it's relatively fast

I checked the logs and I believe this time the problem should be circumented. We fare better by using the cache action explicitly instead of using setup-go built in functionality

@vroldanbet vroldanbet requested a review from a team October 28, 2022 18:22
@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Oct 28, 2022
@vroldanbet vroldanbet marked this pull request as draft October 28, 2022 18:36
@vroldanbet vroldanbet force-pushed the ci-cache-poisoning-redux branch 2 times, most recently from 5085cd5 to 0eb245a Compare October 28, 2022 18:45
@vroldanbet vroldanbet self-assigned this Oct 28, 2022
@vroldanbet vroldanbet marked this pull request as ready for review October 28, 2022 18:49
@vroldanbet vroldanbet enabled auto-merge October 28, 2022 18:50
@vroldanbet vroldanbet force-pushed the ci-cache-poisoning-redux branch from f0327e7 to 305f2c4 Compare November 3, 2022 09:59
vroldanbet and others added 4 commits November 3, 2022 17:04
@vroldanbet
also prevent poisoning in build
b0fb18e 
building only downloads the deps
needed by non-test code, which is a smaller
subset.

We fix it by calling go mod download before
go build, which will get all dependencies
@vroldanbet
do not cache protobuf
c547239 
since it downloads a handful of go modules
@vroldanbet
disable caching in wasm tests
0c927de 
since it downloads a subset of modules
@vroldanbet
explicitly document why cache is disabled
557e6b6
@vroldanbet vroldanbet force-pushed the ci-cache-poisoning-redux branch from 0f14f05 to 557e6b6 Compare November 3, 2022 17:04
josephschorr
josephschorr approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@josephschorr josephschorr left a comment

Choose a reason for hiding this comment

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

LGTM

@vroldanbet vroldanbet merged commit 0520846 into main Nov 3, 2022
@vroldanbet vroldanbet deleted the ci-cache-poisoning-redux branch November 3, 2022 17:11
@github-actions github-actions bot locked and limited conversation to collaborators Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephschorr josephschorr josephschorr approved these changes

Assignees

@vroldanbet vroldanbet

Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI cache poisoning
2 participants
@vroldanbet @josephschorr