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

Support Jib on Google Cloud Build #1478

Merged
merged 3 commits into from
Jan 17, 2019
Merged

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jan 16, 2019

Requires a new schema version (v1beta3)

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #1478 into master will increase coverage by 0.04%.
The diff coverage is 56.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1478      +/-   ##
==========================================
+ Coverage    44.8%   44.84%   +0.04%     
==========================================
  Files         112      115       +3     
  Lines        4638     4696      +58     
==========================================
+ Hits         2078     2106      +28     
- Misses       2350     2372      +22     
- Partials      210      218       +8
Impacted Files Coverage Δ
pkg/skaffold/schema/v1beta1/upgrade.go 58.62% <ø> (ø) ⬆️
pkg/skaffold/schema/versions.go 65.85% <ø> (ø) ⬆️
pkg/skaffold/build/gcb/desc.go 81.25% <0%> (-12.5%) ⬇️
pkg/skaffold/schema/v1beta2/config.go 100% <100%> (ø)
pkg/skaffold/build/gcb/jib.go 100% <100%> (ø)
pkg/skaffold/schema/defaults/defaults.go 43.58% <28.57%> (-5.43%) ⬇️
pkg/skaffold/schema/v1beta2/upgrade.go 58.62% <58.62%> (ø)

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 69776b1...c5c98c5. Read the comment docs.

@dgageot
Copy link
Contributor Author

dgageot commented Jan 16, 2019

Looks like the kokoro failure is not related. Or is it?

@@ -59,6 +59,8 @@ const (
UpdateCheckEnvironmentVariable = "SKAFFOLD_UPDATE_CHECK"

DefaultCloudBuildDockerImage = "gcr.io/cloud-builders/docker"
DefaultCloudBuildMavenImage = "gcr.io/cloud-builders/mvn"
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about using the gcr.io/k8s-skaffold/skaffold image for every builder?

Then we wouldn't have to use multiple images

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy the context + skaffold.yaml, then invoke skaffold build and you get all the builders for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know about that. I have a few questions:

  • Is skaffold pre-pulled on GCB?
  • How would you build only one artifact with skaffold build?
  • Isn't that a bit too inception-ish?

Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
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.

This PR as is LGTM, I'm still not sure how I feel about your comment @r2d4 so I'll let you two figure that out. Up to you whether you want to merge this as is or wait to make that requested change. We could always change it later on too.

cloudbuild "google.golang.org/api/cloudbuild/v1"
)

// TODO(dgageot): check that `package` is bound to `jib:build`
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still TODO or can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still a TODO that might or might not be important

@dgageot dgageot merged commit f302cbe into GoogleContainerTools:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants