Skip to content

Fix unnecessary warning in caching #1873

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

Merged
merged 4 commits into from
Mar 27, 2019

Conversation

priyawadhwa
Copy link
Contributor

If an image is built remotely, we don't want to try to retag/push it locally, otherwise we get an unnecessary warning that caching may not work.

(I know we haven't finished deciding what to do with caching for spring cleanup yet, but we need this fix for the demo me and @dgageot are working on!) :)

If an image is built remotely, we don't want to try to retag/push it locally, otherwise we get an unnecessary warning that caching may not work.

(I know we haven't finished deciding what to do with caching for spring cleanup yet, but we need this fix for the demo me and @dgageot are working on!) :)
@priyawadhwa
Copy link
Contributor Author

thanks @balopat !

@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

Merging #1873 into master will increase coverage by 0.08%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1873      +/-   ##
==========================================
+ Coverage   50.16%   50.24%   +0.08%     
==========================================
  Files         168      168              
  Lines        7356     7354       -2     
==========================================
+ Hits         3690     3695       +5     
+ Misses       3310     3300      -10     
- Partials      356      359       +3
Impacted Files Coverage Δ
pkg/skaffold/build/cache/cache.go 67.34% <100%> (+1.38%) ⬆️
pkg/skaffold/build/cache/retrieve.go 86.82% <100%> (ø) ⬆️
pkg/skaffold/build/cache/save.go 20.68% <33.33%> (+9.97%) ⬆️
pkg/skaffold/runner/runner.go 61.99% <80%> (+0.31%) ⬆️

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 9e23d32...c267a34. Read the comment docs.

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I understand the urgency around the demo - but I think we shouldn't merge this without putting a test around the failing use case. I might be able to help with that too, if you're okay with me meddling in your PR.

@priyawadhwa
Copy link
Contributor Author

@balopat don't mind at all! I wasn't exactly sure what kind of test to write, since the retagging wasn't failing, it was just unnecessary.

I changed the name of the function to RetagLocalImages to make it clear it should be skipped if images aren't built locally, and put in a unit test for the retagging.

@priyawadhwa priyawadhwa requested a review from balopat March 27, 2019 14:30
@balopat balopat merged commit 5753b8b into GoogleContainerTools:master Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants