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

Allow --no-cache to be passed to docker #2054

Merged
merged 10 commits into from
May 2, 2019
Merged

Allow --no-cache to be passed to docker #2054

merged 10 commits into from
May 2, 2019

Conversation

robertrbruno
Copy link
Contributor

As reference in: #2053

This is my first pull request so please carefully review. Happy to make fixes if I did something wrong.

Added NoCache to docker struct
Allow passing in of --no-cache flag
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

This is a great start.
Thank you.
Can you add tests for the same here pkg/skaffold/docker/image_test.go#L178

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Left a few comments, thanks for contributing!

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved
pkg/skaffold/docker/image.go Outdated Show resolved Hide resolved
@@ -631,6 +631,9 @@ type DockerArtifact struct {
// CacheFrom lists the Docker images used as cache sources.
// For example: `["golang:1.10.1-alpine3.7", "alpine:3.7"]`.
CacheFrom []string `yaml:"cacheFrom,omitempty"`

// pass in --no-cache to docker build to prevent caching
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment needs to be in the form

NoCache blah blah blah. otherwise the tests will complain 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completed

add no-cache test
Make bool and improve doc string
Change for bool
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

one more comment!

pkg/skaffold/docker/image_test.go Outdated Show resolved Hide resolved
changed test for no-cache to be bool value
Fixed documentation
@tejal29
Copy link
Contributor

tejal29 commented May 1, 2019

Looks like 2 of the tests are failing.

  1. continuous-integration/appveyor/pr
    If you look at the logs, by clicking on the Details, you see the test error.
?   	github.com/GoogleContainerTools/skaffold/examples/tagging-with-environment-variables	[no test files]
?   	github.com/GoogleContainerTools/skaffold/hack/new_config_version	[no test files]
?   	github.com/GoogleContainerTools/skaffold/hack/release_notes	[no test files]
--- FAIL: TestSchemas (0.20s)
    main_test.go:37: json schema files are not up to date. Please run `make generate-schemas` and commit the changes.
FAIL
FAIL	github.com/GoogleContainerTools/skaffold/hack/schemas	0.295s
ok  	github.com/GoogleContainerTools/skaffold/integration	0.200s
?   	github.com/GoogleContainerTools/skaffold/integration/examples/bazel	[no test files]
?   	github.com/GoogleContainerTools/skaffold/integration/examples/compose	[no test files]

Please run make generate-schemas and then commit the results.
2. cla/google:
Please sign the google by following steps mentioned in #2054 (comment)

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@robertrbruno
Copy link
Contributor Author

The last commit was actually me but didn't have my username and email configured in git. Is there a way to fix the CLA issue easily? Sorry for all of the issues.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels May 2, 2019
@robertrbruno
Copy link
Contributor Author

Learned way too much about git today lol. Removed the commit with bad author.

robertrbruno and others added 2 commits May 1, 2019 21:41
@codecov-io
Copy link

Codecov Report

Merging #2054 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
+ Coverage   56.04%   56.06%   +0.01%     
==========================================
  Files         179      179              
  Lines        7742     7745       +3     
==========================================
+ Hits         4339     4342       +3     
  Misses       2987     2987              
  Partials      416      416
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100% <ø> (ø) ⬆️
pkg/skaffold/docker/image.go 71.92% <100%> (+0.5%) ⬆️

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 15868fb...f3fae40. Read the comment docs.

@priyawadhwa priyawadhwa added the kokoro:run runs the kokoro jobs on a PR label May 2, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 2, 2019
Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@priyawadhwa priyawadhwa merged commit 339edec into GoogleContainerTools:master May 2, 2019
@robertrbruno
Copy link
Contributor Author

Thanks for the opportunity and all the help. Learned a lot!

@amendoza-navent
Copy link

Hi, sorry but I have a question related to this topic, Does this option set the env var DOCKER_BUILDKIT=1 when docker build is executed? Or is there another way to do with skaffold.yaml? thanks!

@nkubala
Copy link
Contributor

nkubala commented Mar 17, 2021

@amendoza-navent in your skaffold.yaml:

build:
  local:
    useBuildKit: true

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.

8 participants