-
Notifications
You must be signed in to change notification settings - Fork 329
More custom artifact bucket fixes #3615
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
Conversation
6e08c36 to
dabe156
Compare
zhming0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this PR is in draft. But looks good, left some comments
dabe156 to
6b9acb0
Compare
6b9acb0 to
a1cc922
Compare
.buildkite/Dockerfile-e2e
Outdated
| && rm -rf awscliv2.zip aws \ | ||
| && apt-get clean \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cleanups don't seem like much of a win, because the image is only used once...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, good point!
DrJosh9000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
a1cc922 to
a40297e
Compare
Description
In #3607, we partially fixed an issue where artifacts uploaded to a customer-managed artifacts bucket would be prefixed with an extra
/, leading to the artifacts not being downloadable.However, in that PR, there was a case that we missed. This PR adds code to cover the missed case, as well as adding an end-to-end test to verify that the behaviour is working as intended, preventing the issue from recurring.
Context
PS-1470
The original PR
Some infra PRs to create the infra the E2E tests need:
Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go tool gofumpt -extra -w .)Disclosures / Credits
As with the original PR, i used Amp as a tool to investigate the original issue, as well as several testing issues i encountered along the way. In some cases it was useful, and in others it led me on wild goose chases.