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

Failing to delete source archive should not fail GCB builds #5891

Merged
merged 2 commits into from
May 21, 2021

Conversation

briandealwis
Copy link
Member

@dhodun mentioned that an error deleting a source archive from a GCB build failed the overall build.

cleaning up source tar after build: storage: object doesn't exist

This should be logged, but it is not a build failure.

It's unclear how this source archive was deleted. Currently we name the source archives with a randomly-generated ID, so theoretically there could be a collision, though @dhodun was unable to find evidence

This PR makes two changes:

  • it logs any deletion errors rather than fail the build
  • it uses a UUID to name the source archive to avoid collisions

User facing changes (remove if N/A)

  • Google Cloud Builder builds no longer treat errors deleting the source archive as a failure

Follow-up Work (remove if N/A)

@briandealwis briandealwis requested a review from a team as a code owner May 21, 2021 18:09
@google-cla google-cla bot added the cla: yes label May 21, 2021
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #5891 (cdefb86) into master (ff5038a) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5891      +/-   ##
==========================================
- Coverage   70.91%   70.89%   -0.02%     
==========================================
  Files         447      447              
  Lines       16908    16909       +1     
==========================================
- Hits        11990    11988       -2     
- Misses       4028     4030       +2     
- Partials      890      891       +1     
Impacted Files Coverage Δ
pkg/skaffold/build/gcb/cloud_build.go 0.00% <0.00%> (ø)
pkg/skaffold/util/tar.go 50.66% <0.00%> (-5.34%) ⬇️
pkg/skaffold/build/buildpacks/init.go 95.23% <0.00%> (ø)
pkg/skaffold/docker/parse.go 87.14% <0.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff5038a...cdefb86. Read the comment docs.

Copy link
Contributor

@aaron-prindle aaron-prindle left a comment

Choose a reason for hiding this comment

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

LGTM

@briandealwis briandealwis merged commit 52cf288 into GoogleContainerTools:master May 21, 2021
@briandealwis briandealwis deleted the gcprm branch May 21, 2021 18:46
@aaron-prindle
Copy link
Contributor

Also not sure if it is interesting but it seems that cloud_build.go is the only file in skaffold/pkg/skaffold/build/gcb/ that does not have an associated _test.go file. Might make sense to add a tracking issue for more testing here if we see more issues.

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

Successfully merging this pull request may close these issues.

2 participants