Enable multi config support in Skaffold #5160

gsquared94 · 2020-12-17T06:42:30Z

This PR introduces the concept of multiple Skaffold configs. This is a prereq to implementing either Skaffold Modules or Skaffold Multiple Configs.

Summary of changes:

There is no single latest.Pipeline anymore that provides build artifacts, deployers and tests. These methods have been moved into runcontext.RunContext which maintains all referenced configs internally and provides target methods for other modules to utilize.
Local, Gcb and Cluster builders are modified to implement build.PipelineBuilder interface instead of build.Builder.

// PipelineBuilder is an interface for a specific Skaffold config pipeline build type.
// Current implementations are the `local`, `cluster` and `gcb`
type PipelineBuilder interface {

	// PreBuild executes any one-time setup required prior to starting any build on this builder
	PreBuild(ctx context.Context, out io.Writer) error

	// Build returns the `ArtifactBuilder` based on this build pipeline type
	Build(ctx context.Context, out io.Writer, artifact *latest.Artifact) ArtifactBuilder

	// PostBuild executes any one-time teardown required after all builds on this builder are complete
	PostBuild(ctx context.Context, out io.Writer) error

	// Concurrency specifies the max number of builds that can run at any one time. If concurrency is 0, then all builds can run in parallel.
	Concurrency() int

	// Prune removes images built in this pipeline
	Prune(context.Context, io.Writer) error
}

Instead build.BuilderMux implements the build.Builder interface and internally manages all build.PipelineBuilder instances. This simplifies out of order builds between the different pipeline builders.
Previously boolean properties like areImagesLocal, importImage, etc. are now functions on the imageName since they can vary between pipelines.

gsquared94 · 2020-12-17T06:42:54Z

cc. @nkubala @briandealwis

codecov · 2020-12-22T02:50:32Z

Codecov Report

Merging #5160 (5ecea52) into master (056bc1d) will decrease coverage by 0.55%.
The diff coverage is 58.87%.

@@            Coverage Diff             @@
##           master    #5160      +/-   ##
==========================================
- Coverage   72.39%   71.84%   -0.56%     
==========================================
  Files         385      387       +2     
  Lines       13735    13923     +188     
==========================================
+ Hits         9944    10003      +59     
- Misses       3063     3188     +125     
- Partials      728      732       +4

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/delete.go	`57.14% <0.00%> (ø)`
cmd/skaffold/app/cmd/deploy.go	`56.52% <0.00%> (-5.39%)`	⬇️
cmd/skaffold/app/cmd/diagnose.go	`34.78% <0.00%> (ø)`
cmd/skaffold/app/cmd/filter.go	`25.58% <0.00%> (-1.25%)`	⬇️
cmd/skaffold/app/cmd/generate_pipeline.go	`61.53% <0.00%> (ø)`
cmd/skaffold/app/cmd/render.go	`46.42% <0.00%> (ø)`
cmd/skaffold/app/cmd/test.go	`46.66% <0.00%> (-7.18%)`	⬇️
cmd/skaffold/app/cmd/util.go	`65.71% <0.00%> (ø)`
pkg/skaffold/build/build.go	`85.71% <ø> (ø)`
pkg/skaffold/deploy/status/status_check.go	`50.75% <0.00%> (ø)`
... and 53 more

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 056bc1d...5ecea52. Read the comment docs.

pkg/skaffold/instrumentation/meter.go

nkubala

for a 98 file change, that was surprisingly painless to review :) left some comments but in general i'm actually feeling pretty good about this. two questions:

what does the UX look like for providing multiple configs in different directory hierarchies? do things generally work well for relative paths in the skaffold.yamls?
can you explain at a high level why we need the PreBuild and PostBuild functions on the builders? i'm assuming this has something to do with parallel builds not stomping on each other, but documenting why we need this would be nice, especially for reference if/when this code gets touched later.

nkubala · 2020-12-22T17:31:06Z

pkg/skaffold/schema/versions.go

@@ -145,12 +145,12 @@ func IsSkaffoldConfig(file string) bool {
 }

 // ParseConfig reads a configuration file.
-func ParseConfig(filename string) (util.VersionedConfig, error) {
+func ParseConfig(filename string) ([]util.VersionedConfig, error) {
+	// TODO: update this function to parse the input as multiple configs


does this mean that the input to the function would be a directory containing multiple configs? wondering if that's something that would make sense to support.

The input could be multiple files (multiple -f flags support) and each file could define multiple configs (if we decide to do Skaffold Multiple Configs.

got it. i'm still wondering if we might want to support running skaffold on a directory and having it find and use all nested configs, but i think that's out of scope of this PR.

cmd/skaffold/app/cmd/find_configs.go

cmd/skaffold/app/cmd/fix.go

cmd/skaffold/app/cmd/runner.go

nkubala · 2020-12-22T17:38:31Z

cmd/skaffold/app/cmd/runner.go

+func setDefaultDeployer(configs []util.VersionedConfig) bool {
+	// set the default deployer only if no deployer is explicitly specified in any config
+	for _, cfg := range configs {
+		if cfg.(*latest.SkaffoldConfig).Deploy.DeployType != (latest.DeployType{}) {


this applies to several places in the code but i had the thought here: i don't love that we do cfg.(*latest.SkaffoldConfig) all over the place to access fields from the config. how easy would it be to through these into interface fields that util.VersionedConfig implements? then we could do something like cfg.DeployType() instead. just a thought

pkg/skaffold/runner/dev_test.go

pkg/skaffold/runner/new.go

pkg/skaffold/runner/runcontext/context.go

pkg/skaffold/yaml/yaml.go

gsquared94 · 2020-12-22T23:36:32Z

* what does the UX look like for providing multiple configs in different directory hierarchies? do things generally work well for relative paths in the skaffold.yamls?

My idea is to support relative paths from all configs by resolving them immediately while parsing. We can use a yamltag to denote which properties are to be interpreted as file paths. Will do that in a subsequent PR where we actually add multi file support.

* can you explain at a high level why we need the `PreBuild` and `PostBuild` functions on the builders? i'm assuming this has something to do with parallel builds not stomping on each other, but documenting why we need this would be nice, especially for reference if/when this code gets touched later.

Let me know if you think there's a better way of doing this. But some builders like cluster builder do secret setup which needs to happen only once per cluster. Also the local builder executes auto pruning of previous skaffold images after all current builds are complete. Having a single Pre and Post function per pipeline seemed a reasonable way to achieve this.

nkubala

My idea is to support relative paths from all configs by resolving them immediately while parsing.

ok this makes sense to me. i was originally also thinking about supporting running skaffold from an arbitrary directory and collecting all the nested skaffold.yamls under that directory and using them in the run, but after thinking more about it i don't think this is a good idea. imagine running skaffold dev from the root directory of our repo 😮

We can use a yamltag to denote which properties are to be interpreted as file paths.

this will all come down to documentation IMO - if we can demonstrate this in a real world scenario and make it easy to discover the moving parts a user needs to control to get their project working, it should be fine.

let's get this merged now, and start playing around with it 👍

wip

ab85fe6

google-cla bot added the cla: yes label Dec 17, 2020

pull-request-size bot added the size/XXL label Dec 17, 2020

gsquared94 added 3 commits December 19, 2020 22:32

Merge remote-tracking branch 'upstream/master' into multiple-configs

1eb5f4d

fix tests

4985914

revert example change

bbaa66e

gsquared94 requested a review from nkubala December 22, 2020 02:08

Merge remote-tracking branch 'upstream/master' into multiple-configs

1d98fa1

gsquared94 marked this pull request as ready for review December 22, 2020 02:38

gsquared94 requested review from chanseokoh, loosebazooka, yuwenma and a team as code owners December 22, 2020 02:38

fix for non-managed images

1db41c2

IsaacPD reviewed Dec 22, 2020

View reviewed changes

pkg/skaffold/instrumentation/meter.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into multiple-configs

af8ea3d

gsquared94 requested a review from IsaacPD December 22, 2020 17:39

gsquared94 commented Dec 22, 2020

View reviewed changes

pkg/skaffold/instrumentation/meter.go Outdated Show resolved Hide resolved

IsaacPD reviewed Dec 22, 2020

View reviewed changes

pkg/skaffold/instrumentation/meter.go Show resolved Hide resolved

nkubala reviewed Dec 22, 2020

View reviewed changes

gsquared94 added 5 commits December 22, 2020 10:49

Merge remote-tracking branch 'upstream/master' into multiple-configs

0032b7b

fix InitMeterFromConfig

3631640

fix message in fix command

2d41563

address PR feedback

585e3ce

add UTs

ac33eae

gsquared94 requested a review from nkubala December 22, 2020 23:03

rename Pipeline to PipelineForImage

5ecea52

nkubala approved these changes Dec 23, 2020

View reviewed changes

gsquared94 merged commit ef6ec47 into GoogleContainerTools:master Dec 23, 2020

This was referenced Dec 28, 2020

Add yaml tag filepath that converts marked fields to absolute paths. #5194

Closed

Allow multiple configs in single skaffold.yaml #5199

Merged

gsquared94 mentioned this pull request Jan 6, 2021

Add go tag filepath that converts marked fields to absolute paths #5205

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable multi config support in Skaffold #5160

Enable multi config support in Skaffold #5160

gsquared94 commented Dec 17, 2020 •

edited

Loading

gsquared94 commented Dec 17, 2020

codecov bot commented Dec 22, 2020 •

edited

Loading

nkubala left a comment

nkubala Dec 22, 2020

gsquared94 Dec 22, 2020

nkubala Dec 22, 2020

nkubala Dec 22, 2020

gsquared94 commented Dec 22, 2020

nkubala left a comment

Enable multi config support in Skaffold #5160

Enable multi config support in Skaffold #5160

Conversation

gsquared94 commented Dec 17, 2020 • edited Loading

gsquared94 commented Dec 17, 2020

codecov bot commented Dec 22, 2020 • edited Loading

Codecov Report

nkubala left a comment

Choose a reason for hiding this comment

nkubala Dec 22, 2020

Choose a reason for hiding this comment

gsquared94 Dec 22, 2020

Choose a reason for hiding this comment

nkubala Dec 22, 2020

Choose a reason for hiding this comment

nkubala Dec 22, 2020

Choose a reason for hiding this comment

gsquared94 commented Dec 22, 2020

nkubala left a comment

Choose a reason for hiding this comment

gsquared94 commented Dec 17, 2020 •

edited

Loading

codecov bot commented Dec 22, 2020 •

edited

Loading