Implement defining remote git dependencies in the skaffold configuration. #5361

gsquared94 · 2021-02-05T20:25:38Z

Description

Implements defining remote git repositories as skaffold config dependencies.

apiVersion: skaffold/v2beta12
kind: Config
requires:
- git:
    repo: https://github.com/GoogleContainerTools/skaffold
    path: examples/multi-config-microservices/leeroy-app
- git:
    repo: https://github.com/GoogleContainerTools/skaffold
    path: examples/multi-config-microservices/leeroy-web

The environment variable SKAFFOLD_REMOTE_CACHE_DIR or flag --remote-cache-dir specifies the download location for all remote repos. If undefined then it defaults to ~/.skaffold/repos.
The repo root directory name is a hash of the repo uri and the branch/ref.
Every execution of a remote module resets the cached repo to the referenced ref. The default ref is master. If master is not defined then it defaults to main.
The remote config gets treated like a local config after substituting the path with the actual path in the cache directory.

Look at the new remote-multi-config-microservices example

…ion.

codecov · 2021-02-05T20:38:58Z

Codecov Report

Merging #5361 (66d3564) into master (7b3d715) will decrease coverage by 0.03%.
The diff coverage is 63.39%.

@@            Coverage Diff             @@
##           master    #5361      +/-   ##
==========================================
- Coverage   71.58%   71.55%   -0.04%     
==========================================
  Files         394      395       +1     
  Lines       14305    14409     +104     
==========================================
+ Hits        10240    10310      +70     
- Misses       3309     3333      +24     
- Partials      756      766      +10

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/flags.go	`87.50% <ø> (ø)`
pkg/skaffold/config/options.go	`100.00% <ø> (ø)`
pkg/skaffold/schema/latest/config.go	`58.82% <ø> (ø)`
pkg/skaffold/git/gitutil.go	`62.96% <62.96%> (ø)`
cmd/skaffold/app/cmd/parse_config.go	`69.81% <64.51%> (-3.69%)`	⬇️
pkg/skaffold/tags/paths.go	`71.66% <0.00%> (+3.33%)`	⬆️
pkg/skaffold/util/tar.go	`56.00% <0.00%> (+5.33%)`	⬆️

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 7b3d715...66d3564. Read the comment docs.

MarlonGamez

Code and testing is looking really good. Have some comments on the example readme

integration/examples/remote-multi-config-microservices/README.md

MarlonGamez

LGTM, thanks for fixing the example :) I won't merge yet as I'm not sure if we want to get another pair of eyes on this since it's a larger PR

nkubala

Every execution of a remote module resets the cached repo to the referenced ref

let's say I have one git module in my project. I run skaffold dev, and make a bunch of local changes to the cloned code which I'm seeing be redeployed to my cluster. I take a break by hitting ctrl+c, and then later start up a new skaffold dev session - all of the changes I made in my previous session are overwritten because my local cache got reset to the remote ref!

I think this should be configurable. it probably makes sense for this to be reset in some cases, but in others users might be experimenting with some changes to a dependency that they later want to commit, or at least persist. the default behavior here might even be to NOT reset the local cache to the remote ref.

cmd/skaffold/app/cmd/parse_config.go

gsquared94 · 2021-02-09T00:19:51Z

Every execution of a remote module resets the cached repo to the referenced ref

let's say I have one git module in my project. I run skaffold dev, and make a bunch of local changes to the cloned code which I'm seeing be redeployed to my cluster. I take a break by hitting ctrl+c, and then later start up a new skaffold dev session - all of the changes I made in my previous session are overwritten because my local cache got reset to the remote ref!

To decide on this behavior I think we need to decide what role a remote dependency serves. IMO it's a central store of Skaffold managed repositories. There can be multiple projects sharing the same repository akin to how a go module gets downloaded and shared by projects using them. IMO if I intend to make modification to a code and keep them I'd rather not have them in that managed store and clone them explicitly myself.

If I selectively allow reset, then I can't have multiple skaffold projects sharing this cache. WDYT?

Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>

gsquared94 · 2021-02-09T01:32:13Z

Every execution of a remote module resets the cached repo to the referenced ref

let's say I have one git module in my project. I run skaffold dev, and make a bunch of local changes to the cloned code which I'm seeing be redeployed to my cluster. I take a break by hitting ctrl+c, and then later start up a new skaffold dev session - all of the changes I made in my previous session are overwritten because my local cache got reset to the remote ref!

To decide on this behavior I think we need to decide what role a remote dependency serves. IMO it's a central store of Skaffold managed repositories. There can be multiple projects sharing the same repository akin to how a go module gets downloaded and shared by projects using them. IMO if I intend to make modification to a code and keep them I'd rather not have them in that managed store and clone them explicitly myself.

If I selectively allow reset, then I can't have multiple skaffold projects sharing this cache. WDYT?

cc. @tejal29, @briandealwis, @marlon-gamez what do you guys think about this?

nkubala · 2021-02-09T17:17:03Z

IMO if I intend to make modification to a code and keep them I'd rather not have them in that managed store and clone them explicitly myself

one of my thoughts around this was that this feature would be a good way to "trial" out a service in your application, since it's really easy to just plug-and-play any service with a skaffold.yaml. in that case we might want to allow users to make some local modifications without stomping on their changes - but the more i think about this, the more i think this is an uncommon use case.

my opinion is that we should give the ability to users to turn this auto-syncing off with a config field, but we should definitely default to resetting their local changes on every run. you make a great point that these local clones could be shared across projects, in which case local changes could cause unexpected behavior.

MarlonGamez · 2021-02-09T20:39:43Z

I'm in agreement with what Gaurav is saying, and I also agree with Nick that this should be configurable as well.

cmd/skaffold/app/cmd/parse_config.go

briandealwis · 2021-02-09T22:11:15Z

cmd/skaffold/app/cmd/parse_config.go

+	configNameToFile map[string]string      // configName -> file path
+	cachedRepos      map[string]interface{} // git repo -> cache path or error
+}
+
 func getAllConfigs(opts config.SkaffoldOptions) ([]*latest.SkaffoldConfig, error) {


Would it make sense to have these functions be methods on record?

I don't think there's any difference either way.

pkg/skaffold/git/gitutil.go

cmd/skaffold/app/cmd/parse_config.go

pkg/skaffold/schema/latest/config.go

pkg/skaffold/git/gitutil_test.go

cmd/skaffold/app/cmd/parse_config.go

briandealwis · 2021-02-10T14:42:52Z

I agree with @gsquared94: the local clones in ~/.skaffold/repos are shared and should be treated as read-only otherwise you risk interfering with other builds.

But @nkubala is right too: people need the ability make tweaks and overrides (viz go.mod's replace). Especially when using multi-level dependencies. Users can do local changes by using a different --remote-cache-dir (maybe this should be renamed to --git-remotes?). I wonder if we should include the ability for a top-level config to do go.mod-like replacements to override a dependency.

So I'd advocate changing the strict behaviour to logging warnings on finding unsaved work or unpublished commits.

Implement defining remote git dependencies in the skaffold configurat…

de6540f

…ion.

gsquared94 requested review from tejal29 and nkubala February 5, 2021 20:25

gsquared94 requested a review from a team as a code owner February 5, 2021 20:25

google-cla bot added the cla: yes label Feb 5, 2021

pull-request-size bot added the size/XL label Feb 5, 2021

add more UTs

9eafde9

MarlonGamez requested changes Feb 5, 2021

View reviewed changes

integration/examples/remote-multi-config-microservices/README.md Outdated Show resolved Hide resolved

fix README.md

029ac38

gsquared94 requested a review from MarlonGamez February 8, 2021 16:59

MarlonGamez approved these changes Feb 8, 2021

View reviewed changes

nkubala reviewed Feb 8, 2021

View reviewed changes

cmd/skaffold/app/cmd/parse_config.go Outdated Show resolved Hide resolved

Update cmd/skaffold/app/cmd/parse_config.go

aba607f

Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>

gsquared94 added 2 commits February 9, 2021 20:30

add sync property controlling repository reset.

ee9420f

fix tests

95bbfc3

gsquared94 requested a review from nkubala February 10, 2021 06:05

briandealwis reviewed Feb 10, 2021

View reviewed changes

small refactor; change gitutil.go boilerplate.

66d3564

gsquared94 added the kokoro:force-run forces a kokoro re-run on a PR label Feb 10, 2021

kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Feb 10, 2021

gsquared94 merged commit 0ba3ee7 into GoogleContainerTools:master Feb 10, 2021

fmotrifork mentioned this pull request Feb 12, 2021

skaffold 1.20.0 fishworks/fish-food#1223

Merged

gsquared94 added the feature/multi-configs label Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement defining remote git dependencies in the skaffold configuration. #5361

Implement defining remote git dependencies in the skaffold configuration. #5361

gsquared94 commented Feb 5, 2021 •

edited

codecov bot commented Feb 5, 2021 •

edited

MarlonGamez left a comment

MarlonGamez left a comment •

edited

nkubala left a comment

gsquared94 commented Feb 9, 2021

gsquared94 commented Feb 9, 2021

nkubala commented Feb 9, 2021

MarlonGamez commented Feb 9, 2021

briandealwis Feb 9, 2021

gsquared94 Feb 10, 2021

briandealwis commented Feb 10, 2021

Implement defining remote git dependencies in the skaffold configuration. #5361

Implement defining remote git dependencies in the skaffold configuration. #5361

Conversation

gsquared94 commented Feb 5, 2021 • edited

codecov bot commented Feb 5, 2021 • edited

Codecov Report

MarlonGamez left a comment

Choose a reason for hiding this comment

MarlonGamez left a comment • edited

Choose a reason for hiding this comment

nkubala left a comment

Choose a reason for hiding this comment

gsquared94 commented Feb 9, 2021

gsquared94 commented Feb 9, 2021

nkubala commented Feb 9, 2021

MarlonGamez commented Feb 9, 2021

briandealwis Feb 9, 2021

Choose a reason for hiding this comment

gsquared94 Feb 10, 2021

Choose a reason for hiding this comment

briandealwis commented Feb 10, 2021

gsquared94 commented Feb 5, 2021 •

edited

codecov bot commented Feb 5, 2021 •

edited

MarlonGamez left a comment •

edited