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

Use viper to read create template params from a config #2235

Merged
merged 11 commits into from
Jan 18, 2023

Conversation

sarataha
Copy link
Member

@sarataha sarataha commented Jan 17, 2023

Closes #2227

What changed?
Added viper to read create template params from a config file instead of setting them manually.

Why was this change made?
The command requires writing many parameters when running it.

How was this change implemented?

How did you validate the change?
Manually + unit tests.

Release notes

Documentation Changes

@sarataha sarataha changed the title Add --config flag to create templates to set flags using a file Use viper to read create template params from a config Jan 17, 2023
@sarataha sarataha added team/pesto enhancement New feature or request labels Jan 17, 2023
foot and others added 4 commits January 17, 2023 17:01
- Read configFile into a separate var, it can't be inside the config
- Unmarshal struct has to have public properties apparently
@sarataha sarataha marked this pull request as ready for review January 17, 2023 19:19
Copy link
Collaborator

@foot foot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ and with tests! ✨

- NAMESPACE=test-namespace
- GIT_REPO_NAMESPACE=test-git-repo-namespace
- GIT_REPO_NAME=test-git-repo-name
- PATH=../clusters/out.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spf13/viper#411 :(, Had a quick play w/ StringToString but we end up with case-insensitive keys here which is a shame. This might be the best we can do without writing lots of parsing stuff.

Copy link
Member Author

@sarataha sarataha Jan 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we could extract these parameters from the template file and compare its lower case version to the received parameter and swap the keys, and if it's actually necessary 😅

@foot
Copy link
Collaborator

foot commented Jan 18, 2023

e.g.

diff --git a/cmd/gitops/app/create/templates/cmd.go b/cmd/gitops/app/create/templates/cmd.go
index 5b78dba1..2c32f796 100644
--- a/cmd/gitops/app/create/templates/cmd.go
+++ b/cmd/gitops/app/create/templates/cmd.go
@@ -22,10 +22,10 @@ import (
 )
 
 type Config struct {
-	ParameterValues []string `mapstructure:"values"`
-	Export          bool     `mapstructure:"export"`
-	OutputDir       string   `mapstructure:"output-dir"`
-	TemplateFile    string   `mapstructure:"template-file"`
+	ParameterValues map[string]string `mapstructure:"values"`
+	Export          bool              `mapstructure:"export"`
+	OutputDir       string            `mapstructure:"output-dir"`
+	TemplateFile    string            `mapstructure:"template-file"`
 }
 
 var config Config
@@ -49,7 +49,7 @@ var CreateCommand = &cobra.Command{
 
 func init() {
 	flags := CreateCommand.Flags()
-	flags.StringSlice("values", []string{}, "Set parameter values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")
+	flags.StringToString("values", map[string]string{}, "Set parameter values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")
 	flags.Bool("export", false, "export in YAML format to stdout")
 	flags.String("output-dir", "", "write YAML format to file")
 	flags.String("template-file", "", "template file to use")
@@ -102,18 +102,9 @@ func templatesCmdRunE() func(*cobra.Command, []string) error {
 		}
 
 		ctx := context.Background()
-		vals := make(map[string]string)
-
-		// parse parameter values
-		for _, v := range config.ParameterValues {
-			kv := strings.SplitN(v, "=", 2)
-			if len(kv) == 2 {
-				vals[kv[0]] = kv[1]
-			}
-		}
 
 		getFilesRequest := server.GetFilesRequest{
-			ParameterValues: vals,
+			ParameterValues: config.ParameterValues,
 			TemplateName:    parsedTemplate.Name,
 			TemplateKind:    parsedTemplate.Kind,
 		}
diff --git a/cmd/gitops/app/create/templates/cmd_test.go b/cmd/gitops/app/create/templates/cmd_test.go
index e95e4cf1..6a3294f9 100644
--- a/cmd/gitops/app/create/templates/cmd_test.go
+++ b/cmd/gitops/app/create/templates/cmd_test.go
@@ -106,9 +106,14 @@ func Test_initializeConfig(t *testing.T) {
 
 	assert.Equal(t, config.TemplateFile, "template.yaml")
 
-	expectedParams := []string{
-		"CLUSTER_NAME=test-cluster", "RESOURCE_NAME=test-resource", "NAMESPACE=test-namespace",
-		"GIT_REPO_NAMESPACE=test-git-repo-namespace", "GIT_REPO_NAME=test-git-repo-name", "PATH=../clusters/out.yaml"}
+	expectedParams := map[string]string{
+		"CLUSTER_NAME":       "test-cluster",
+		"RESOURCE_NAME":      "test-resource",
+		"NAMESPACE":          "test-namespace",
+		"GIT_REPO_NAMESPACE": "test-git-repo-namespace",
+		"GIT_REPO_NAME":      "test-git-repo-name",
+		"PATH":               "../clusters/out.yaml",
+	}
 
 	if diff := cmp.Diff(expectedParams, config.ParameterValues); diff != "" {
 		t.Fatalf("result didn't match expected:\n%s", diff)
diff --git a/cmd/gitops/app/create/templates/testdata/config.yaml b/cmd/gitops/app/create/templates/testdata/config.yaml
index ef47dd7e..a4b9ee85 100644
--- a/cmd/gitops/app/create/templates/testdata/config.yaml
+++ b/cmd/gitops/app/create/templates/testdata/config.yaml
@@ -1,8 +1,8 @@
 template-file: template.yaml
 values:
-  - CLUSTER_NAME=test-cluster
-  - RESOURCE_NAME=test-resource
-  - NAMESPACE=test-namespace
-  - GIT_REPO_NAMESPACE=test-git-repo-namespace
-  - GIT_REPO_NAME=test-git-repo-name
-  - PATH=../clusters/out.yaml
\ No newline at end of file
+  CLUSTER_NAME: test-cluster
+  RESOURCE_NAME: test-resource
+  NAMESPACE: test-namespace
+  GIT_REPO_NAMESPACE: test-git-repo-namespace
+  GIT_REPO_NAME: test-git-repo-name
+  PATH: ../clusters/out.yaml

Fails with:

Error: must specify template file
--- FAIL: Test_initializeConfig (0.00s)
    /Users/simon/weave/weave-gitops-enterprise/cmd/gitops/app/create/templates/cmd_test.go:119: result didn't match expected:
          map[string]string{
        - 	"CLUSTER_NAME":       "test-cluster",
        - 	"GIT_REPO_NAME":      "test-git-repo-name",
        - 	"GIT_REPO_NAMESPACE": "test-git-repo-namespace",
        - 	"NAMESPACE":          "test-namespace",
        - 	"PATH":               "../clusters/out.yaml",
        - 	"RESOURCE_NAME":      "test-resource",
        + 	"cluster_name":       "test-cluster",
        + 	"git_repo_name":      "test-git-repo-name",
        + 	"git_repo_namespace": "test-git-repo-namespace",
        + 	"namespace":          "test-namespace",
        + 	"path":               "../clusters/out.yaml",
        + 	"resource_name":      "test-resource",
          }
FAIL
coverage: 42.0% of statements
FAIL	github.com/weaveworks/weave-gitops-enterprise/cmd/gitops/app/create/templates	0.450s
FAIL

@foot
Copy link
Collaborator

foot commented Jan 18, 2023

So still LGTM! 🎉

@sarataha sarataha merged commit cad2491 into main Jan 18, 2023
@sarataha sarataha deleted the template-flags-config branch January 18, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request team/pesto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gitops create template requires you to type too many params
2 participants