Rename SkaffoldConfig to SkaffoldPipeline #1087
Conversation
pkg/skaffold/runner/runner_test.go
Outdated
| @@ -155,15 +155,15 @@ func (t *TestWatcher) Run(ctx context.Context, pollInterval time.Duration, onCha | |||
| func TestNewForConfig(t *testing.T) { | |||
| var tests = []struct { | |||
| description string | |||
| config *latest.SkaffoldConfig | |||
| config *latest.SkaffoldPipeline | |||
There was a problem hiding this comment.
nit: rename config -> pipeline
cmd/skaffold/app/cmd/init.go
Outdated
| // if we're here, the user has no skaffold yaml so we need to generate one | ||
| // if the user doesn't have any k8s yamls, generate one for each dockerfile | ||
| logrus.Info("generating skaffold config") | ||
|
|
||
| config := &latest.SkaffoldConfig{ | ||
| config := &latest.SkaffoldPipeline{ |
| @@ -231,14 +231,13 @@ func processBuildArtifacts(pairs []dockerfilePair) latest.BuildConfig { | |||
| return config | |||
There was a problem hiding this comment.
I left BuildConfig and DeployConfig as they are, WDYT?
cmd/skaffold/app/cmd/init.go
Outdated
| @@ -129,7 +129,7 @@ func doInit(out io.Writer) error { | |||
| } | |||
| } | |||
|
|
|||
| cfg, err := generateSkaffoldConfig(k8sConfigs, pairs) | |||
| cfg, err := generateSkaffoldPipeline(k8sConfigs, pairs) | |||
| // NewForConfig returns a new SkaffoldRunner for a SkaffoldConfig | ||
| func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldConfig) (*SkaffoldRunner, error) { | ||
| // NewForConfig returns a new SkaffoldRunner for a SkaffoldPipeline | ||
| func NewForConfig(opts *config.SkaffoldOptions, cfg *latest.SkaffoldPipeline) (*SkaffoldRunner, error) { |
There was a problem hiding this comment.
I think its going to be a lot more confusing if functions like NewForConfig takes in a latest.SkaffoldPipeline
There was a problem hiding this comment.
We can always rename NewForConfig to NewForPipeline
@r2d4 the conversation started in #1057 (comment) - I think "global config" is a poor name - local config, global config ... it makes it very confusing to navigate the code (just like the 3 configs are in minikube).
Maybe. If this is the only thing that makes a yaml a k8s resource then we can introduce it later. If there are other things - like metadata - we don't have those yet as they are not used for anything else...I'm not sure I'm convinced that we need this right now. |
This is mostly internal terminology, you can still refer to your skaffold.yaml as your "skaffold config", and your ~/.skaffold/config as your "global config". The clash between the global config and the skaffold config file is getting a little confusing so I want to try and solve it. We could rename the global config to the "global preferences", but I'm not really a huge fan of that. It to me seems like more of a config file than the skaffold.yaml, which is really just specifying the pipeline that Skaffold should execute when it runs. Hence, the |
Codecov Report
@@ Coverage Diff @@
## master #1087 +/- ##
==========================================
+ Coverage 43.16% 43.17% +0.01%
==========================================
Files 74 74
Lines 3408 3407 -1
==========================================
Hits 1471 1471
+ Misses 1799 1798 -1
Partials 138 138
Continue to review full report at Codecov.
|
|
I added the |
|
@balopat I'm not sure I like |
|
I'm ok with the renaming. Not that I really thought it was required |
This change is to eliminate internal confusion between the configuration specified in the
skaffold.yaml, and for the global configuration values set through theskaffold configcommand.This also removes theKind: Configfield from theskaffold.yaml, since it serves no purpose.