feat: allow ValuesFile from GCS#9182
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@JeromeJu can you review this one? |
|
@JeromeJu can you review this one? Please let me know if any other action is to be taken care. |
|
|
||
| //if the file starts with gs:// then download it in tmp dir | ||
| if strings.HasPrefix(v, gcsPrefix) { | ||
| tempDir, err := os.MkdirTemp("", valueFileFromGCS) |
There was a problem hiding this comment.
the condition's body can be extracted for better readability
There was a problem hiding this comment.
Thanks for the suggestion, yes separated to the new function.
pkg/skaffold/helm/args.go
Outdated
| tempValueFile = filepath.Join(tempDir, path.Base(v)) | ||
|
|
||
| gcs := gcs.Gsutil{} | ||
| if err := gcs.Copy(context.Background(), v, tempValueFile, false); err != nil { |
There was a problem hiding this comment.
why context.Background(), but not TODO()?
There was a problem hiding this comment.
I updated to TODO() as it requires the context temporarily
| tempValueFile := v | ||
|
|
||
| //if the file starts with gs:// then download it in tmp dir | ||
| if strings.HasPrefix(v, gcsPrefix) { |
There was a problem hiding this comment.
it looks like it'll be better to contribute to the helm repository
There was a problem hiding this comment.
Skaffold has already been integrated with GCP, so I thought to make the change here.
|
@idsulik can you review this? |
|
@ashokhein I did it, you need to wait for the maintainers |
|
@Darien-Lin Can you please review this code? |
|
Can you add tests for this? Im going to talk with the team, as this seems to have implications that might be unexpected. Findings: Cloud Deploy works with local files only and will not be able to take advantage of referencing gs buckets. However, it already cannot take advantage of gs buckets for other files like raw manifests. This should be a no-op for Cloud Deploy. Or at least, should behave consistently with the other fields that reference gs uris. |
Darien-Lin
left a comment
Darien-Lin
left a comment
There was a problem hiding this comment.
Thank you for adding the tests. I have started the presubmits for this PR.
Can you please address the linting issues found by the linter? Thanks for the contribution!
I fixed that linting issue. Thank you. |
3 participants
Fixes: #9172
Description
Allow to reference the ValuesFile from Google Storage Bucket. It uses the gsutil to copy the remote file and, put it in the temporary directory and reference it in the helm flags.