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

feat: allow specifying a remote config dependency from Google Cloud Storage #9057

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

mattsanta
Copy link
Contributor

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.

…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".
@mattsanta mattsanta marked this pull request as draft August 30, 2023 16:38
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Merging #9057 (66c0671) into main (290280e) will decrease coverage by 7.13%.
Report is 1020 commits behind head on main.
The diff coverage is 49.04%.

@@            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     
Files Changed Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <0.00%> (ø)
... and 40 more

... and 425 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mattsanta mattsanta changed the title Allow specifying a remote config dependency from Google Cloud Storage feat: allow specifying a remote config dependency from Google Cloud Storage Aug 30, 2023
@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Aug 30, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Aug 30, 2023
@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Aug 30, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Aug 30, 2023
@mattsanta mattsanta marked this pull request as ready for review August 31, 2023 12:21
@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Sep 1, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Sep 1, 2023
@@ -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
Copy link
Contributor

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(?)

Copy link
Contributor Author

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!

@renzodavid9
Copy link
Contributor

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 🤔

@mattsanta
Copy link
Contributor Author

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.

@renzodavid9 renzodavid9 added the kokoro:force-run forces a kokoro re-run on a PR label Sep 5, 2023
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Sep 5, 2023
@renzodavid9
Copy link
Contributor

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!

@renzodavid9 renzodavid9 merged commit 5943bd5 into GoogleContainerTools:main Sep 5, 2023
14 checks passed
@mattsanta mattsanta deleted the new branch September 6, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants