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

skaffold render --output takes GCS file path #3979

Merged
merged 9 commits into from
Apr 27, 2020

Conversation

ChrisGe4
Copy link
Contributor

Fixes: #3902

Description

This PR adds gsutil.go that calls 'gsutil' to copy rendered manifests to a GCS bucket when skaffold render --output takes a file path starts with gs://

@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #3979 into master will decrease coverage by 0.04%.
The diff coverage is 61.29%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/util.go 65.62% <33.33%> (-13.55%) ⬇️
pkg/skaffold/deploy/deploy_mux.go 89.13% <100.00%> (ø)
pkg/skaffold/deploy/kubectl.go 66.40% <100.00%> (ø)
pkg/skaffold/util/gsutil.go 100.00% <100.00%> (ø)

@dgageot
Copy link
Contributor

dgageot commented Apr 21, 2020

Hi @ChrisGe4, I'm a bit reluctant in merging such feature that avoids piping into gsutil. Can you tell us more about why you need this feature?

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Apr 21, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 21, 2020
@ChrisGe4
Copy link
Contributor Author

Hi @dgageot, we are interested in using Skaffold in a new project. One step in the workflow is uploading the hydrated manifests to a GCS bucket.
I choose to use gsutil because I found skaffold is directly calling kubectl so I used the same approach. I also had a meeting with several skaffold team members earlier and they had no objection to install gsutil in the skaffold cloud builder in the future. Those are the reasons why I created this PR.

pkg/skaffold/util/gsutil.go Outdated Show resolved Hide resolved
pkg/skaffold/util/gsutil.go Outdated Show resolved Hide resolved
pkg/skaffold/util/gsutil_test.go Show resolved Hide resolved
pkg/skaffold/deploy/util.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/util.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/util.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/util.go Outdated Show resolved Hide resolved
@dgageot
Copy link
Contributor

dgageot commented Apr 23, 2020

@ChrisGe4 ok. I'm still a bit reluctant since it looks like we could just pipe the output of skaffold to gsutil. But let's move on! I added a few comments.

@dgageot dgageot self-assigned this Apr 23, 2020
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.

LGTM with nit

pkg/skaffold/util/gsutil_test.go Outdated Show resolved Hide resolved
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.

LGTM also with two tiny nits

pkg/skaffold/util/gsutil_test.go Outdated Show resolved Hide resolved
pkg/skaffold/util/gsutil.go Outdated Show resolved Hide resolved
@nkubala
Copy link
Contributor

nkubala commented Apr 23, 2020

@ChrisGe4 looks like you actually have one small linter error, can you check that out? not sure what happened with the kokoro job but it looks like it got stuck so I restarted it. when CI is green I'll merge

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Apr 27, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 27, 2020
@dgageot dgageot merged commit c21327c into GoogleContainerTools:master Apr 27, 2020
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.

5 participants