Add skaffold 'init' by nkubala · Pull Request #919 · GoogleContainerTools/skaffold

nkubala · 2018-08-22T18:41:08Z

This adds a new CLI command, 'init', for initializing
repositories with basic skaffold configuration.
Skaffold will prompt the users for some basic configuration
options, and then autogenerate a skaffold.yaml with sane defaults.

skaffold init will find all valid Dockerfiles in the source directory, all valid k8s manifests in the directory, and parse out all image names from the k8s manifests. The default behavior will then be to prompt the user for which Dockerfile builds each image (since we can't infer this ourselves), though these pairs can also be passed through a repeated --artifact flag.

By default, this will write the generated skaffold.yaml to stdout, though a --file flag can be passed to write the generated config to a file.

An optional --skip-build flag can be passed to bypass generating build artifacts for the config.

Demo using the microservices example:

dgageot · 2018-08-22T18:54:03Z

@nkubala linter is failing

nkubala · 2018-08-22T19:52:49Z

@dgageot I guess my local golangci-lint version was old, but the hack/linter.sh script doesn't fail because of it. Should be fixed now

r2d4 · 2018-08-22T20:03:13Z

Gopkg.toml

  revision = "0c8571ac0ce161a5feb57375a9cdf148c98c0f70"

 [[constraint]]
-  name = "github.com/moby/buildkit"


looks like a bad merge resolution here, you'll have to add these back in?

Ugh, yeah somehow this is hitting dep issues even though it builds locally for me. Working on it

FYI these should be resolved now. I think we're fine removing these and letting dep figure resolve the correct version based on transitive dependencies

r2d4 · 2018-08-22T20:03:34Z

Gopkg.toml

+
+[[override]]
+  name = "github.com/docker/docker"
+  revision = "71cd53e4a197b303c6ba086bd584ffd67a884281"


is 71cd53e4a197b303c6ba086bd584ffd67a884281 newer than 162ba6016def672690ee4a1f3978368853a1e149?

r2d4 · 2018-08-22T20:05:12Z

cmd/skaffold/app/cmd/init.go

+func AddInitFlags(cmd *cobra.Command) {
+	cmd.Flags().StringVarP(&outfile, "file", "f", "", "File to write generated skaffold config")
+	cmd.Flags().BoolVar(&skipBuild, "skip-build", false, "Skip generating build artifacts in skaffold config")
+	cmd.Flags().StringArrayVarP(&cliArtifacts, "artifact", "a", nil, "'='-delimited dockerfile/image pair to generate build artifact\n(example: --artifact=/web/Dockerfile.web=gcr.io/web-project/image)")


you can consider making this its own flagtype, like the TemplateFlag

r2d4 · 2018-08-22T20:06:59Z

cmd/skaffold/app/cmd/init.go

+	}
+}
+
+func generateSkaffoldConfig(k8sConfigs []string, dockerfilePairs []dockerfilePair) ([]byte, error) {


Can you add an integration test to make sure the config that we generate is actually a valid and runnable config? Something like

skaffold init .. (input options) skaffold run

Yep, tests to come

r2d4 · 2018-08-22T20:10:13Z

cmd/skaffold/app/cmd/init.go

+		if f.IsDir() {
+			return nil
+		}
+		if util.IsSupportedKubernetesFormat(path) {


This will return JSON files as well

That should be ok. Skaffold will first try and parse it as a Dockerfile (this will fail), then it will try and parse it as a skaffold config (this will also fail), then finally as a kubernetes manifest, which should succeed. I just realized that my code doesn't try and read the k8s manifests as JSON though, let me fix that

r2d4 · 2018-08-22T20:12:21Z

pkg/skaffold/docker/image.go

 	}

-	img, err := remote.Image(ref, auth, tr)
+	img, err := remote.Image(ref, remote.WithAuth(auth), remote.WithTransport(tr))


was this meant to be changed?

Yeah, this was a change I made in the go-containerregistry code that then got picked up here when I updated the version. Shouldn't have any effect.

codecov-io · 2018-08-27T22:32:45Z

Codecov Report

Merging #919 into master will decrease coverage by 3.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
- Coverage   43.94%   40.87%   -3.07%     
==========================================
  Files          64       66       +2     
  Lines        2667     2867     +200     
==========================================
  Hits         1172     1172              
- Misses       1373     1573     +200     
  Partials      122      122

Impacted Files	Coverage Δ
pkg/skaffold/docker/image.go	`59.18% <0%> (ø)`	⬆️
pkg/skaffold/util/util.go	`40.69% <0%> (-2.52%)`	⬇️
cmd/skaffold/app/cmd/cmd.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/config/config.go	`0% <0%> (ø)`
cmd/skaffold/app/cmd/init.go	`0% <0%> (ø)`
pkg/skaffold/docker/parse.go	`66.66% <0%> (-3.87%)`	⬇️

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 cfe5a70...614305f. Read the comment docs.

r2d4 · 2018-08-29T16:34:47Z

integration/run_test.go

+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {


if you do skaffold run -f - you can pipe in the skaffold yaml over stdin!

if only I had been a clever boy, I would have done this

r2d4 · 2018-08-29T16:35:31Z

integration/run_test.go

+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			oldYamlPath := filepath.Join(test.dir, "skaffold.yaml")
+			oldYaml, err := removeOldSkaffoldYaml(oldYamlPath)


or you could just use skaffold run -f skaffold.yaml.out

that's what I'm doing :) the reason I had to do this funky business is because the example directories all have skaffold.yamls in them, and skaffold init won't run if it detects a valid skaffold config in your directory. this just temporarily removes it so we can autogenerate one, then pass it to skaffold with skaffold run -f <generatedYaml>

balopat

bug: the Dockerfile lister picks up files and directories starting with ".":

See on @ahmetb's microservices-demo:

$ ~/go/src/github.com/GoogleContainerTools/skaffold/out/skaffold init
? Choose the dockerfile to build image gcr.io/****/cartservice  [Use arrows to move, type to filter]
❯ .git/info/exclude
  src/cartservice/.vs/cartservice/v15/Server/sqlite3/db.lock
  src/cartservice/Dockerfile

idea: we could probably introduce a heuristic to find the Dockerfile for the image using a "longest common substring" matching on the path and the imagename - this feels like we could do more than just listing all the Dockerfiles when there are a lot of them (could be a new issue!):

$ ~/go/src/github.com/GoogleContainerTools/skaffold/out/skaffold init
? Choose the dockerfile to build image gcr.io/****/cartservice  [Use arrows to move, type to filter]
 src/cartservice/.vs/cartservice/v15/Server/sqlite3/db.lock
❯ src/cartservice/Dockerfile
  src/checkoutservice/Dockerfile
  src/currencyservice/Dockerfile
  src/emailservice/Dockerfile
  src/frontend/.gitkeep

bug: for some reason shippingservice comes up twice, and I can't choose the second one:

 ~/go/src/github.com/GoogleContainerTools/skaffold/out/skaffold init
? Choose the dockerfile to build image gcr.io/******/cartservice src/cartservice/Dockerfile
? Choose the dockerfile to build image gcr.io/******/currencyservice src/currencyservice/Dockerfile
? Choose the dockerfile to build image gcr.io/******/frontend src/frontend/Dockerfile
? Choose the dockerfile to build image gcr.io/******/loadgenerator src/loadgenerator/Dockerfile
? Choose the dockerfile to build image gcr.io/******/productcatalogservice src/productcatalogservice/Dockerfile
? Choose the dockerfile to build image redis:alpine None (image not built from these sources)
? Choose the dockerfile to build image gcr.io/******/shippingservice src/shippingservice/Dockerfile
? Choose the dockerfile to build image gcr.io/******/shippingservice  [Use arrows to move, type to filter]
  .git/info/exclude
  src/cartservice/.vs/cartservice/v15/Server/sqlite3/db.lock
  src/checkoutservice/Dockerfile
❯ src/emailservice/Dockerfile
  src/frontend/.gitkeep
  src/paymentservice/Dockerfile
  src/recommendationservice/Dockerfile

This adds a new CLI command, 'init', for initializing repositories with basic skaffold configuration. Skaffold will prompt the users for some basic configuration options, and then autogenerate a skaffold.yaml with sane defaults.

…nd fix image loop when processing dockerfiles

nkubala · 2018-09-11T23:19:59Z

@balopat thanks for finding those issues! I addressed 1 and 3, I think I'll leave 2 to a separate issue (though I definitely think we should do it, for the microservices example it would be very useful). PTAL

balopat

LGTM :)

nkubala requested review from balopat, dgageot and r2d4 as code owners August 22, 2018 18:41

nkubala requested a review from dlorenc August 22, 2018 18:41

r2d4 reviewed Aug 22, 2018

View reviewed changes

nkubala force-pushed the init branch 2 times, most recently from d82c392 to d650fd9 Compare August 28, 2018 21:55

r2d4 reviewed Aug 29, 2018

View reviewed changes

balopat suggested changes Sep 6, 2018

View reviewed changes

nkubala added 8 commits September 10, 2018 13:52

Add skaffold 'init'

847eb24

This adds a new CLI command, 'init', for initializing repositories with basic skaffold configuration. Skaffold will prompt the users for some basic configuration options, and then autogenerate a skaffold.yaml with sane defaults.

add flag to write config out to file

36f77f0

add skip-build flag to bypass generating build config

e51eec0

add repeated --artifact flag for passing build artifacts through CLI

2bbfc5c

linter

6405fd6

use moby/moby dockerfile parser instead of kaniko

58f4f30

fix workspace/dockerfile paths in generated config

78d3e73

add integration tests

b93be93

nkubala force-pushed the init branch from 4c9b044 to b93be93 Compare September 10, 2018 21:08

ignore directories starting with '.', fix dockerfile parsing logic, a…

614305f

…nd fix image loop when processing dockerfiles

nkubala force-pushed the init branch from acbbbf7 to 614305f Compare September 11, 2018 23:19

balopat approved these changes Sep 12, 2018

View reviewed changes

nkubala merged commit abe8032 into GoogleContainerTools:master Sep 12, 2018

nkubala deleted the init branch September 12, 2018 16:40

Conversation

nkubala commented Aug 22, 2018

Uh oh!

dgageot commented Aug 22, 2018

Uh oh!

nkubala commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balopat left a comment • 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!

nkubala commented Sep 11, 2018

Uh oh!

balopat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Aug 27, 2018 •

edited

Loading

balopat left a comment •

edited

Loading