Global skaffold config #896

nkubala · 2018-08-13T20:46:07Z

This is a second variant on #886. Main differences:

changes the structure of the config to have a top-level "global" section, followed by any number of context-specific config entries, inspired by @balopat. Example:

global: 
  default-repo: gcr.io/myrepo1/
kubecontexts:
   - kubectx: minikube
         default_repo: gcr.io/myrepo2/
   - kubectx: my_dev_gke
         default_repo: gcr.io/myrepo2/

Setting global values in the config is done through the --global flag, i.e. skaffold config set default-repo my-global-repo --global.

disallows setting config values for multiple contexts at a time
instead of specifying --kubecontext all to show all config values, a --all flag is added to the list command. skaffold config list --all

Opening this PR to show what this implementation would look like.

codecov-io · 2018-08-23T19:34:51Z

Codecov Report

Merging #896 into master will decrease coverage by 0.29%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master     #896     +/-   ##
=========================================
- Coverage   43.58%   43.28%   -0.3%     
=========================================
  Files          62       63      +1     
  Lines        2611     2629     +18     
=========================================
  Hits         1138     1138             
- Misses       1354     1372     +18     
  Partials      119      119

Impacted Files	Coverage Δ
pkg/skaffold/util/util.go	`43.2% <0%> (-6.09%)`	⬇️
cmd/skaffold/app/cmd/config.go	`0% <0%> (ø)`
cmd/skaffold/app/cmd/cmd.go	`0% <0%> (ø)`	⬆️

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 64c97a9...b4bee87. Read the comment docs.

balopat · 2018-08-23T23:51:33Z

cmd/skaffold/app/cmd/config_test.go

+	}
+}
+
+// func TestSetConfig(t *testing.T) {


do you want to remove this block or still hack on it?

whoops, nope I wrote this as an integration test so I'll remove it

r2d4 · 2018-08-23T23:57:40Z

cmd/skaffold/app/cmd/config/flags.go

+
+func AddConfigFlags(cmd *cobra.Command) {
+	cmd.Flags().StringVarP(&configFile, "config", "c", "", "path to skaffold config")
+	cmd.Flags().StringVarP(&kubectx, "kubectx", "k", "", "kubectl context to set values against")


re: ahmet's comment from before that this isn't a standard term, just the name of his tool. change to kube-context?

yeah I'm fine with that

r2d4 · 2018-08-23T23:57:50Z

cmd/skaffold/app/cmd/config/config.go

+}
+
+type ContextConfig struct {
+	Kubectx     string `yaml:"kubectx,omitempty"`


in the config also?

r2d4 · 2018-08-23T23:58:45Z

cmd/skaffold/app/cmd/config/util.go

+	}
+	_, err = os.Stat(configFile)
+	// TODO(nkubala): create default config?
+	if err != nil {


should check if os.IsNotExist or if regular error

decided to use os.OpenFile here to create a default if it doesn't exist

r2d4 · 2018-08-24T00:00:07Z

integration/run_test.go

+	}
+}
+
+func randomString() string {


how about https://github.com/GoogleContainerTools/skaffold/blob/master/pkg/skaffold/util/util.go#L33?

r2d4 · 2018-08-24T00:02:44Z

Gopkg.lock

@@ -1160,10 +1152,10 @@
    "github.com/pkg/errors",
    "github.com/sirupsen/logrus",
    "github.com/spf13/cobra",
+    "github.com/spf13/pflag",


where is this dependency coming from?

this is just an artifact added by the dep solver. the actual pflag dependency didn't change here

r2d4 · 2018-08-24T00:03:33Z

cmd/skaffold/app/cmd/config/util.go

+	yaml "gopkg.in/yaml.v2"
+)
+
+const defaultConfigLocation = ".skaffold/config"


will this work on windows?

it should, I see windows-specific logic in github.com/mitchellh/go-homedir which is what I'm using here along with filepath.Join

Do you need to do a filepath.Join() on this? Should it be .skaffold\config on windows?

good catch, I should have thought about that when I responded :)

…previously exist

…ing global config entry

balopat

I played around with the UX:

if the folder is not created it throws errors in all the subcommands
no list for possible configs - would be helpful instead of just providing a "this is not a default config"
no feedback around which context I set the value for - it would be great to have the feedback when I set the config regarding what I changed as it can be surprising

nkubala · 2018-08-27T18:30:39Z

@balopat

if the folder is not created it throws errors in all the subcommands

Folder now gets autocreated along with config file if it doesn't exist

no list for possible configs - would be helpful instead of just providing a "this is not a default config"

Most other config management CLIs don't do this (kubectl, gcloud, etc.) since it requires introspecting about the config struct at runtime. I could add this but I think it's probably better to just document it somewhere and use that as a reference.

no feedback around which context I set the value for - it would be great to have the feedback when I set the config regarding what I changed as it can be surprising

Updated config value with context is now logged to the user

… user

balopat · 2018-08-29T21:25:35Z

@nkubala - this lgtm - let's wait for @dgageot and @r2d4 with the merge

r2d4 · 2018-08-29T21:31:41Z

cmd/skaffold/app/cmd/config/set.go

+		RunE: func(cmd *cobra.Command, args []string) error {
+			resolveKubectlContext()
+			err := setConfigValue(args[0], args[1])
+			if err == nil {


might be more idiomatic to do

if err != nil { return err } logConfigChangeForUser(out, args[0], args[1])

I actually had this before and changed it, because it's one less line to do it this way: the way you described you also need to do a return nil afterwards. curious to hear your opinion on this

slight preference for the return nil, but up to you

r2d4 · 2018-08-29T21:32:47Z

cmd/skaffold/app/cmd/config/util.go

+	yaml "gopkg.in/yaml.v2"
+)
+
+const defaultConfigLocation = ".skaffold/config"


Do you need to do a filepath.Join() on this? Should it be .skaffold\config on windows?

r2d4 · 2018-08-29T21:33:18Z

cmd/skaffold/app/cmd/config/util.go

+	context, err := context.CurrentContext()
+	if err != nil {
+		logrus.Warn(errors.Wrap(err, "retrieving current kubectl context"))
+		kubecontext = "default"


add this as a constant somewhere?

actually I reworked this logic to fall back to the global values (not a "default" context) if one isn't set, thanks for pointing this out. updated

r2d4 · 2018-08-29T21:34:19Z

cmd/skaffold/app/cmd/config/util.go

+func resolveConfigFile() error {
+	if configFile != "" {
+		// we had a config provided as a flag, expand it and return
+		if !filepath.IsAbs(configFile) {


why does this need to be an absolute path? add the reasoning as a comment?

thanks, this was from some previous logic which I removed a while back so I'll take this out

r2d4 · 2018-08-29T21:41:53Z

cmd/skaffold/app/cmd/config/config.go

+
+package config
+
+type Config struct {


add a docstring here so its clear this is unrelated to SkaffoldConfig

nkubala added 3 commits August 7, 2018 16:51

add skaffold config list/set commands

1ae6092

review comments

f6d5f48

[WIP] Global skaffold config, version 2

410cd9f

nkubala requested review from balopat, dgageot and r2d4 as code owners August 13, 2018 20:46

nkubala mentioned this pull request Aug 13, 2018

[WIP] Global skaffold config, version 3 #897

Closed

balopat mentioned this pull request Aug 21, 2018

Add global skaffold config with list/set commands #886

Closed

lint

7593f24

nkubala force-pushed the global_config_v2 branch from a8b0c8c to 7593f24 Compare August 23, 2018 19:45

tests

20b9866

nkubala changed the title ~~[WIP] Global skaffold config, version 2~~ Global skaffold config Aug 23, 2018

balopat self-assigned this Aug 23, 2018

balopat suggested changes Aug 23, 2018

View reviewed changes

r2d4 reviewed Aug 24, 2018

View reviewed changes

nkubala added 2 commits August 24, 2018 11:28

review comments, and small fix for writing config entry that doesn't …

dce2c12

…previously exist

use os.OpenFile over os.Stat to create config if not exist. fix creat…

4c7d8ac

…ing global config entry

nkubala force-pushed the global_config_v2 branch from a45dcf0 to 4c7d8ac Compare August 24, 2018 20:42

balopat suggested changes Aug 25, 2018

View reviewed changes

create config folder if it doesn't exist. log updated config value to…

b9faaf7

… user

nkubala force-pushed the global_config_v2 branch from 6dec2f1 to b9faaf7 Compare August 27, 2018 18:32

balopat approved these changes Aug 29, 2018

View reviewed changes

r2d4 self-assigned this Aug 29, 2018

r2d4 approved these changes Aug 29, 2018

View reviewed changes

cleanup a few things and address final comments:

b4bee87

nkubala force-pushed the global_config_v2 branch from 49a84f2 to b4bee87 Compare August 30, 2018 00:52

nkubala merged commit 61a96ad into GoogleContainerTools:master Aug 30, 2018

nkubala deleted the global_config_v2 branch August 30, 2018 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Global skaffold config #896

Global skaffold config #896

nkubala commented Aug 13, 2018

codecov-io commented Aug 23, 2018 •

edited

Loading

balopat Aug 23, 2018

nkubala Aug 24, 2018

r2d4 Aug 23, 2018

nkubala Aug 24, 2018

r2d4 Aug 23, 2018

nkubala Aug 24, 2018

r2d4 Aug 23, 2018

nkubala Aug 24, 2018

r2d4 Aug 24, 2018

nkubala Aug 24, 2018

r2d4 Aug 24, 2018

nkubala Aug 24, 2018

r2d4 Aug 24, 2018

nkubala Aug 24, 2018

r2d4 Aug 29, 2018

nkubala Aug 29, 2018

balopat left a comment

nkubala commented Aug 27, 2018

balopat commented Aug 29, 2018

r2d4 Aug 29, 2018

nkubala Aug 29, 2018

r2d4 Aug 29, 2018

r2d4 Aug 29, 2018

r2d4 Aug 29, 2018

nkubala Aug 29, 2018

r2d4 Aug 29, 2018

nkubala Aug 29, 2018

r2d4 Aug 29, 2018

nkubala Aug 29, 2018

Global skaffold config #896

Global skaffold config #896

Conversation

nkubala commented Aug 13, 2018

codecov-io commented Aug 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balopat left a comment

Choose a reason for hiding this comment

nkubala commented Aug 27, 2018

balopat commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 23, 2018 •

edited

Loading