Add custom artifact for custom local builds by priyawadhwa · Pull Request #1999 · GoogleContainerTools/skaffold

priyawadhwa · 2019-04-22T22:04:22Z

This is the first PR for custom builders in skaffold. This PR adds in support for:

Building a custom artifact using the local builder
I added unit tests, an integration test, and tested locally with a GKE cluster and minikube

Upcoming PRs will add support for:

Building a custom artifact using the cluster builder
Specifying dockerfile, bazel, jib or a command for retrieving dependencies

…o custom

Pass in docker env variables so that built images are built directly into minikube. Also add to the unit test and update the build script to only push images if necessary.

…uilders

Created a custom artifact builder which local/cluster builders will call out to for building custom artifacts. Added unit tests.

codecov-io · 2019-04-30T17:43:17Z

Codecov Report

Merging #1999 into master will decrease coverage by 0.09%.
The diff coverage is 48.43%.

@@            Coverage Diff            @@
##           master    #1999     +/-   ##
=========================================
- Coverage   56.03%   55.93%   -0.1%     
=========================================
  Files         175      178      +3     
  Lines        7615     7689     +74     
=========================================
+ Hits         4267     4301     +34     
- Misses       2940     2973     +33     
- Partials      408      415      +7

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100% <ø> (ø)`	⬆️
pkg/skaffold/build/local/local.go	`37.31% <0%> (-2.37%)`	⬇️
pkg/skaffold/build/local/custom.go	`0% <0%> (ø)`
pkg/skaffold/build/custom/custom.go	`51.72% <51.72%> (ø)`
pkg/skaffold/docker/parse.go	`78.5% <60%> (-1.21%)`	⬇️
pkg/skaffold/build/custom/dependencies.go	`77.77% <77.77%> (ø)`
pkg/skaffold/build/gcb/docker.go	`90.47% <0%> (-9.53%)`	⬇️
pkg/skaffold/build/local/docker.go	`53.19% <0%> (-3.63%)`	⬇️
pkg/skaffold/docker/image.go	`71.42% <0%> (-0.7%)`	⬇️
... and 3 more

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 11f8a1b...c3339ce. Read the comment docs.

integration/testdata/custom/build.sh

pkg/skaffold/schema/latest/config.go

balopat · 2019-04-30T18:50:42Z

pkg/skaffold/schema/latest/config.go

+type CustomArtifact struct {
+	// BuildCommand is the command executed to build the image.
+	BuildCommand string `yaml:"buildCommand,omitempty"`
+	// Dependencies are the file dependencies that skaffold should watch for this artifact.


Suggested change

// Dependencies are the file dependencies that skaffold should watch for this artifact.

// Dependencies are the file dependencies that skaffold should watch for both rebuilding and file syncing for this artifact.

pkg/skaffold/schema/latest/config.go

balopat

Thank you @priyawadhwa, this will be awesome!

I did a quick "drive-by" review and it looks good to me, based on all the discussions we had.

More documentation would be great - a section with ALPHA warnings on the How-to/Builders should describe how to use customArtifact builders, what the contract is between the scripts and skaffold and how they impact file sync, logging, etc.

tejal29

This looks really good except more documentation on the ArtifactBuilder.

tejal29 · 2019-04-30T18:50:33Z

pkg/skaffold/build/local/custom.go

+	}
+
+	if b.pushImages {
+		return docker.RemoteDigest(tag, b.insecureRegistries)


What if skaffold pushes the image to Remote instead of build script here?

So the custom build script is responsible for pushing. We pass in pushImages to the build script so that it knows if the image is expected to exist remotely or locally.

no my question was around "skaffold actually pushing an image here instead of retrieving it". Since custom artifact builder is local builder the build script shd have build it in local docker.
But i think after looking at what other buildesr do there are builder which directly build into registry.
So i change my mind.

pkg/skaffold/build/custom/custom.go

container-tools-bot · 2019-04-30T20:44:34Z

Please visit http://35.236.22.24:1313 to view changes to the docs.

priyawadhwa · 2019-04-30T20:45:19Z

@balopat @tejal29 added documentation!

container-tools-bot · 2019-04-30T20:47:49Z

Please visit http://35.236.22.24:1313 to view changes to the docs.

docs/content/en/docs/how-tos/builders/_index.md

tejal29 · 2019-04-30T21:31:01Z

docs/content/en/docs/how-tos/builders/_index.md

+##### Local builder
+| Environment Variable         | Description           | Expectation  |
+| ------------- |-------------| -----|
+| Docker daemon environment variables     | Inform the custom builder of which docker daemon endpoint we are using. Allows custom build scripts to work with tools like Minikube.| None. | 


is there a doc on what these environment variables are? The closet i found is https://docs.docker.com/machine/reference/env/

Please add a link here.

docs/content/en/docs/how-tos/builders/_index.md

tejal29

LGTM except for doc nits.

Priya Wadhwa added 3 commits April 22, 2019 12:00

wip

dd35ad8

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

4bfc75d

…o custom

prototype for custom builders works

257181e

googlebot added the cla: yes label Apr 22, 2019

Make build script more general, add bazel directories to gitignore

0c51b3b

tejal29 mentioned this pull request Apr 23, 2019

Deploy to consume build output #2001

Merged

This was referenced Apr 23, 2019

Add CustomArtifact to local builder to support arbitrary build scripts #2009

Closed

WIP: add please build #1631

Closed

Priya Wadhwa added 2 commits April 24, 2019 11:38

Merge branch 'master' of github.com:GoogleContainerTools/skaffold int…

f55ea8a

…o custom

remove dockerfile dependency for now

8b90ffb

priyawadhwa force-pushed the custom branch from f52ec4d to 82d244c Compare April 24, 2019 18:52

add unit test for retrieving environ

7b2ec3b

priyawadhwa force-pushed the custom branch from 82d244c to 7b2ec3b Compare April 24, 2019 19:03

Priya Wadhwa added 6 commits April 24, 2019 15:20

get started making sure it works with minikube

6b3cd54

Make sure custom artifact works w/ minikube

95babe2

Pass in docker env variables so that built images are built directly into minikube. Also add to the unit test and update the build script to only push images if necessary.

add custom builder integration test

3ed255c

merged master, fixed merge conflict in constants

ac17c52

update schema to v1beta10, since we are adding in fields for custom b…

a03ca29

…uilders

move custom to testdata

67eeb83

priyawadhwa force-pushed the custom branch from c9bb26b to 67eeb83 Compare April 29, 2019 18:11

Priya Wadhwa added 4 commits April 29, 2019 13:54

rebase on master

1caa16f

Create custom artifact builder

156ae70

Created a custom artifact builder which local/cluster builders will call out to for building custom artifacts. Added unit tests.

Pass in additional envs to custom artifact builder from local builder

624b0f7

update build.sh to reflect change from IMAGE_NAME to IMAGES

e59589b

priyawadhwa marked this pull request as ready for review April 30, 2019 00:01

priyawadhwa requested review from balopat, dgageot, nkubala and tejal29 as code owners April 30, 2019 00:01

priyawadhwa changed the title ~~[WIP] Add custom artifact for custom local builds~~ Add custom artifact for custom local builds Apr 30, 2019

fix linting error & simplify custom artifact build signature

87aae86

add unit test for get dependencies

f1fae32

priyawadhwa force-pushed the custom branch from da1cd64 to f1fae32 Compare April 30, 2019 18:42

tejal29 reviewed Apr 30, 2019

View reviewed changes

integration/testdata/custom/build.sh Show resolved Hide resolved

balopat reviewed Apr 30, 2019

View reviewed changes

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved

balopat reviewed Apr 30, 2019

View reviewed changes

pkg/skaffold/schema/latest/config.go Outdated Show resolved Hide resolved

balopat reviewed Apr 30, 2019

View reviewed changes

tejal29 reviewed Apr 30, 2019

View reviewed changes

code review comments and added documentation

05a5ee8

priyawadhwa added the docs-modifications runs the docs preview service on the given PR label Apr 30, 2019

container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 30, 2019

priyawadhwa closed this Apr 30, 2019

priyawadhwa reopened this Apr 30, 2019

priyawadhwa added the docs-modifications runs the docs preview service on the given PR label Apr 30, 2019

container-tools-bot removed the docs-modifications runs the docs preview service on the given PR label Apr 30, 2019

tejal29 reviewed Apr 30, 2019

View reviewed changes

tejal29 approved these changes Apr 30, 2019

View reviewed changes

Final code review comments for docs

c3339ce

priyawadhwa merged commit 4a4d696 into GoogleContainerTools:master May 1, 2019

priyawadhwa deleted the custom branch May 1, 2019 00:15

priyawadhwa added the build/custom label May 8, 2019

	// Dependencies are the file dependencies that skaffold should watch for this artifact.
	// Dependencies are the file dependencies that skaffold should watch for both rebuilding and file syncing for this artifact.

Conversation

priyawadhwa commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

balopat Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

tejal29 left a comment

Choose a reason for hiding this comment

Uh oh!

tejal29 Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

priyawadhwa Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

tejal29 Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

container-tools-bot commented Apr 30, 2019

Uh oh!

priyawadhwa commented Apr 30, 2019

Uh oh!

container-tools-bot commented Apr 30, 2019

Uh oh!

Uh oh!

tejal29 Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tejal29 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

priyawadhwa commented Apr 22, 2019 •

edited

Loading

codecov-io commented Apr 30, 2019 •

edited

Loading

tejal29 Apr 30, 2019 •

edited

Loading

tejal29 Apr 30, 2019 •

edited

Loading