Skip to content

Simplify skaffold init code #3406

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 9 commits into from
Dec 20, 2019
Merged

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Dec 19, 2019

Each commit is a simplification. I didn't merge them to ease the review.
Also fixes #3405

@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #3406 into master will increase coverage by 0.06%.
The diff coverage is 85.54%.

Impacted Files Coverage Δ
pkg/skaffold/docker/docker_init.go 100% <100%> (ø) ⬆️
pkg/skaffold/build/jib/init.go 88.13% <100%> (ø) ⬆️
pkg/skaffold/initializer/init.go 63.41% <68.57%> (+2.79%) ⬆️
pkg/skaffold/build/buildpacks/init.go 86.66% <90.9%> (-4.25%) ⬇️

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.

One issue with these changes is that it will generate a bit more verbose skaffold.yaml than necessary based on our current defaults - however there is no tests ensuring this behavior yet, and it is not the end of the world. But still a bit of a regression in UX in my opinion:

diff --git a/examples/microservices/skaffold.yaml b/examples/microservices/skaffold.yaml
index f4e408246..4bd5ba5b3 100644
--- a/examples/microservices/skaffold.yaml
+++ b/examples/microservices/skaffold.yaml
@@ -1,18 +1,22 @@
-apiVersion: skaffold/v1
+apiVersion: skaffold/v2alpha1
 kind: Config
+metadata:
+  name: microservices
 build:
   artifacts:
-    - image: gcr.io/k8s-skaffold/leeroy-web
-      context: ./leeroy-web/
-    - image: gcr.io/k8s-skaffold/leeroy-app
-      context: ./leeroy-app/
+  - image: gcr.io/k8s-skaffold/leeroy-app
+    context: leeroy-app
+    docker:
+      dockerfile: Dockerfile
+  - image: gcr.io/k8s-skaffold/leeroy-web
+    context: leeroy-web
+    docker:
+      dockerfile: Dockerfile
+  tagPolicy:
+    gitCommit: {}
+  local: {}
...

vs the current version:

diff --git a/examples/microservices/skaffold.yaml b/examples/microservices/skaffold.yaml
index f4e408246..b79fcbb74 100644
--- a/examples/microservices/skaffold.yaml
+++ b/examples/microservices/skaffold.yaml
@@ -1,18 +1,15 @@
-apiVersion: skaffold/v1
+apiVersion: skaffold/v2alpha1
 kind: Config
+metadata:
+  name: microservices
 build:
   artifacts:
-    - image: gcr.io/k8s-skaffold/leeroy-web
-      context: ./leeroy-web/
-    - image: gcr.io/k8s-skaffold/leeroy-app
-      context: ./leeroy-app/
+  - image: gcr.io/k8s-skaffold/leeroy-app
+    context: leeroy-app
+  - image: gcr.io/k8s-skaffold/leeroy-web
+    context: leeroy-web

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.

LGTM with the nit that this version of skaffold init doesn't respect default value setting. I would rather keep the yaml minimal when possible. WDYT?

@dgageot
Copy link
Contributor Author

dgageot commented Dec 19, 2019

Good catch @balopat!

It used to be that only a few defaults were explicit.
Now they all are. I don't really like that,
I was thinking of having none...

Anyways, I think it’s best to add a non regression test and make sure we produce the same yaml as before. Wdyt?

@dgageot
Copy link
Contributor Author

dgageot commented Dec 20, 2019

I changed a few things to make sure we produce the same yaml as before.

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.

Nice! Good to go! Thank you for the improvements, I believe this is a much better UX to keep the yaml minimal!

Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
@dgageot dgageot merged commit 25f77f1 into GoogleContainerTools:master Dec 20, 2019
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.

skaffold init can produce an erroneous config with non-default Dockerfile name
3 participants