-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: allow specifying a remote config dependency from Google Cloud Storage #9057
Conversation
…more inline with git. Instead of copying the skaffold config from GCS the user specifies a source that signifies the bucket and directory structure to copy recursively. For example, specifying the source "gs://my-bucket/dir1/*" will copy all the files and subdirectories stored in "gs://my-bucket/dir1/". This requires the user to also specify the path to the skaffold config relative to the provided source. In the example mentioned, if the skaffold config was stored in "gs://my-bucket/dir1/configs/skaffold.yaml" then the path required would be "configs/skaffold.yaml".
Codecov Report
@@ Coverage Diff @@
## main #9057 +/- ##
==========================================
- Coverage 70.48% 63.35% -7.13%
==========================================
Files 515 626 +111
Lines 23150 32161 +9011
==========================================
+ Hits 16317 20376 +4059
- Misses 5776 10245 +4469
- Partials 1057 1540 +483
... and 425 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pkg/skaffold/parser/config.go
Outdated
@@ -231,7 +235,9 @@ func processEachConfig(ctx context.Context, config *latest.SkaffoldConfig, cfgOp | |||
} | |||
} | |||
} | |||
newOpts := configOpts{file: cfgOpts.file, profiles: depProfiles, isRequired: required, isDependency: cfgOpts.isDependency} | |||
isRemoteDependency := d.GitRepo != nil || d.GoogleCloudStorage != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure but, is this needed? I think we are always using the value from the flag in line 282 (isRemoteCfg), and we are overwriting the value from here(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, removed. Thanks!
Thanks @mattsanta! Just one more small check. Also I was thinking that these remote dependencies implementations (Git and GCS) are sharing some logic, (e.g, if we should download or not the dependency according to the different flags values and current state of the remote cache folder), do you think we should find a way to unify everything? I think is ok if we don't do it in this PR, but just curious about your opinion 🤔 |
Yeah, I think this would be valuable especially as more remote dependencies are added as well. To be honest, I wanted to limit the amount of changes that I made in a new code base that I'm unfamiliar with and felt like this already had a good amount of changes. |
Yeap, I agree, this could be a improvement for the future then. Thanks! |
Related: #9056
Description
Introduces the ability to specify a remote config dependency that is stored in Google Cloud Storage (GCS). The current feature only supports remote configs stored in git repositories.
User facing changes (remove if N/A)
Skaffold config has new fields to support configuring the GCS remote config dependency. Also updated the docs on the feature at https://skaffold.dev/docs/design/config/#remote-config-dependency.