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

Implement required artifact resolution for cluster builder #4992

Merged
merged 6 commits into from Nov 5, 2020

Conversation

gsquared94
Copy link
Collaborator

@gsquared94 gsquared94 commented Nov 3, 2020

Related: #4713, #4922

Description (Part 6 of several PRs.)

This PR implements resolving required artifacts as build args in the kaniko and custom cluster builder as described in the design for supporting dependencies between build artifacts.

A lot of files show as modified since I had to modify the signature of function docker.EvalBuildArgs to reuse with the KanikoArtifact:

-    func evalBuildArgs(mode config.RunMode, workspace string, a *latest.DockerArtifact, extra map[string]*string) (map[string]*string, error)
+    func evalBuildArgs(mode config.RunMode, workspace string, dockerfilePath string, args map[string]*string, extra map[string]*string) (map[string]*string, error)

The main changes are in cluster.go to add the calculated build args to the KanikoArtifact.

@gsquared94 gsquared94 requested a review from a team as a code owner November 3, 2020 13:23
@google-cla google-cla bot added the cla: yes label Nov 3, 2020
@gsquared94 gsquared94 changed the title Kaniko builder Implement required artifact resolution for cluster builder Nov 3, 2020
@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #4992 into master will decrease coverage by 0.02%.
The diff coverage is 57.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4992      +/-   ##
==========================================
- Coverage   72.16%   72.14%   -0.03%     
==========================================
  Files         363      363              
  Lines       12762    12772      +10     
==========================================
+ Hits         9210     9214       +4     
- Misses       2866     2874       +8     
+ Partials      686      684       -2     
Impacted Files Coverage Δ
pkg/skaffold/build/cluster/cluster.go 22.85% <0.00%> (-3.81%) ⬇️
pkg/skaffold/build/dependencies.go 33.33% <28.57%> (-19.30%) ⬇️
pkg/skaffold/build/docker/docker.go 42.00% <50.00%> (ø)
pkg/skaffold/docker/build_args.go 72.97% <84.61%> (+4.97%) ⬆️
pkg/skaffold/build/cache/hash.go 73.40% <100.00%> (ø)
pkg/skaffold/build/cluster/types.go 100.00% <100.00%> (ø)
pkg/skaffold/build/local/types.go 79.24% <100.00%> (ø)
pkg/skaffold/docker/image.go 81.93% <100.00%> (+1.32%) ⬆️

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 6c9376c...ca71321. Read the comment docs.

@@ -294,7 +294,7 @@ func TestBuildArgs(t *testing.T) {
for _, test := range tests {
testutil.Run(t, "", func(t *testutil.T) {
tmpDir := t.NewTempDir()
tmpDir.Write("./Dockerfile", "ARG SKAFFOLD_GO_GCFLAGS\nFROM foo")
tmpDir.Write("./Dockerfile", "ARG SKAFFOLD_GO_GCFLAGS\nFROM foo\nCMD bar")
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why is the CMD bar necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In #4943 we added additional Dockerfile validation that fails if there isn't atleast one valid command in the Dockerfile.

Copy link
Member

Choose a reason for hiding this comment

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

That needs to be fixed as ENTRYPOINT and CMD are inherited from the base image.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I removed the CMD and tests were fine. I was probably getting error on an empty Dockerfile and I thought this was it.

pkg/skaffold/build/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/skaffold/build/cluster/cluster.go Outdated Show resolved Hide resolved
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.

None yet

2 participants