Skip to content

Add user-friendly validation of builder/artifact compatibility #4312

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

Conversation

tstromberg
Copy link
Contributor

@tstromberg tstromberg commented Jun 11, 2020

Fixes #4161

Example case

If cluster is removed from examples/kaniko/skaffold.yaml, and skaffold build is run.

Old error message

couldn't build "skaffold-example": undefined artifact type: {DockerArtifact:<nil> BazelArtifact:<nil> JibArtifact:<nil> KanikoArtifact:0xc000542be0 BuildpackArtifact:<nil> CustomArtifact:<nil>}

New error message

invalid skaffold config: Found a 'kaniko' artifact, which is incompatible with the 'local' builder:

image: skaffold-example
context: .
kaniko:
  dockerfile: Dockerfile
  initImage: busybox
  image: gcr.io/kaniko-project/executor:v0.20.0@sha256:f9a4a760166682c7c7aeda3cc263570682e00848ab47737ed8ffcc3abd2da6c3
  cache: {}

To use the 'kaniko' builder, add the 'cluster' stanza to the 'build' section of your configuration. For information, see https://skaffold.dev/docs/pipeline-stages/builders/

@tstromberg tstromberg changed the title Add a user-friendly error message if incompatible builder is specified Add user-friendly error message if incompatible builder is used Jun 11, 2020
@codecov
Copy link

codecov bot commented Jun 11, 2020

Codecov Report

Merging #4312 into master will increase coverage by 0.04%.
The diff coverage is 65.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4312      +/-   ##
==========================================
+ Coverage   71.96%   72.00%   +0.04%     
==========================================
  Files         323      324       +1     
  Lines       12370    12404      +34     
==========================================
+ Hits         8902     8932      +30     
- Misses       2904     2905       +1     
- Partials      564      567       +3     
Impacted Files Coverage Δ
pkg/skaffold/build/cluster/cluster.go 27.27% <0.00%> (ø)
pkg/skaffold/build/dependencies.go 0.00% <0.00%> (ø)
pkg/skaffold/build/local/local.go 62.79% <40.00%> (ø)
pkg/skaffold/schema/validation/validation.go 94.95% <64.70%> (-5.05%) ⬇️
pkg/skaffold/build/misc/artifact_type.go 90.47% <90.47%> (ø)
pkg/skaffold/build/gcb/spec.go 100.00% <100.00%> (+5.88%) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 63.41% <0.00%> (+2.43%) ⬆️

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 a92f8f9...636a579. Read the comment docs.

Copy link
Contributor

@dgageot dgageot left a comment

Choose a reason for hiding this comment

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

Would it make sense to move this validation to validation.Process()?

@tstromberg
Copy link
Contributor Author

@dgageot - That seems reasonable. I wasn't aware of the validation package function before, but I could easily move the user-friendly error message there, and leave the internal error where it is now.

@dgageot
Copy link
Contributor

dgageot commented Jun 11, 2020

@tstromberg I think that would make sense

@tstromberg
Copy link
Contributor Author

Done. Please take another look.

@tstromberg tstromberg changed the title Add user-friendly error message if incompatible builder is used Add user-friendly validation for builder/artifact compatibility Jun 11, 2020
@tstromberg tstromberg changed the title Add user-friendly validation for builder/artifact compatibility Add user-friendly validation of builder/artifact compatibility Jun 11, 2020
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

nice, these error messages are way better. small nits but otherwise LGTM

@tstromberg tstromberg merged commit 469af0d into GoogleContainerTools:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants