chore(cli): Adding support for Variable Sets #3018

xoscar · 2023-08-01T17:27:10Z

This PR updates the CLI and fixes all of the automated tests to work with the new variable set changes

Changes

Updates CLI resources from environments to variable sets
Changes the environment flag for the vars flag when used when running a test
Fixes CLI e2e tests and trace-based testing

Fixes

Update the CLI #3014

Checklist

tested locally
added new dependencies
updated the docs
added a test

https://www.loom.com/share/ccd34203368e4b9ba921ce8530cfb258

danielbdias

The overall PR is ok, but I'm missing one CLI e2e test: we should have a scenario to run a test using vars in the old format and the new one, just to guarantee that we are not breaking existing tests for our customers.
One example of test that does that is here: https://github.com/kubeshop/tracetest/blob/main/testing/cli-e2etest/testscenarios/test/run_test_test.go#L40

danielbdias · 2023-08-02T15:46:18Z

cli/cmd/resource_run_cmd.go

@@ -63,7 +63,7 @@ func init() {

 	runCmd.Flags().StringVarP(&runParams.DefinitionFile, "file", "f", "", "path to the definition file")
 	runCmd.Flags().StringVar(&runParams.ID, "id", "", "id of the resource to run")
-	runCmd.Flags().StringVarP(&runParams.EnvID, "environment", "e", "", "environment file or ID to be used")
+	runCmd.Flags().StringVarP(&runParams.VarsID, "vars", "", "", "variable set file or ID to be used")


Does it make sense to be variableset here?

Suggested change

runCmd.Flags().StringVarP(&runParams.VarsID, "vars", "", "", "variable set file or ID to be used")

runCmd.Flags().StringVarP(&runParams.VarsID, "variableset", "", "", "variable set file or ID to be used")

As we were using a shorthand for environments, I thought we could continue using the same approach

xoscar · 2023-08-02T20:19:25Z

cli/cmd/middleware.go

@@ -84,8 +82,8 @@ func (p *resourceParameters) Validate(cmd *cobra.Command, args []string) []error

 	p.ResourceName = args[0]

-	_, err := resources.Get(p.ResourceName)
-	if errors.Is(err, resourcemanager.ErrResourceNotFound) {
+	exists := resources.Exists(p.ResourceName)


I changed this to use a different function because Get now writes the warning to the console if deprecated

xoscar · 2023-08-02T20:19:51Z

cli/cmd/resource_run_cmd.go

@@ -63,7 +63,7 @@ func init() {

 	runCmd.Flags().StringVarP(&runParams.DefinitionFile, "file", "f", "", "path to the definition file")
 	runCmd.Flags().StringVar(&runParams.ID, "id", "", "id of the resource to run")
-	runCmd.Flags().StringVarP(&runParams.EnvID, "environment", "e", "", "environment file or ID to be used")
+	runCmd.Flags().StringVarP(&runParams.VarsID, "vars", "", "", "variable set file or ID to be used")


As we were using a shorthand for environments, I thought we could continue using the same approach

xoscar · 2023-08-02T20:20:06Z

cli/cmd/resource_run_cmd.go

 	runCmd.Flags().BoolVarP(&runParams.SkipResultWait, "skip-result-wait", "W", false, "do not wait for results. exit immediately after test run started")
 	runCmd.Flags().StringVarP(&runParams.JUnitOuptutFile, "junit", "j", "", "file path to save test results in junit format")
 	runCmd.Flags().StringSliceVar(&runParams.RequriedGates, "required-gates", []string{}, "override default required gate. "+validRequiredGatesMsg())
+
+	//deprecated


Still supporting this in case users need it

xoscar · 2023-08-02T20:20:45Z

cli/cmd/resources.go

 		resourcemanager.WithTableConfig(resourcemanager.TableConfig{
 			Cells: []resourcemanager.TableCellConfig{
 				{Header: "ID", Path: "spec.id"},
 				{Header: "NAME", Path: "spec.name"},
 				{Header: "DESCRIPTION", Path: "spec.description"},
 			},
 		}),
+		resourcemanager.WithResourceType("VariableSet"),
+		resourcemanager.WithApplyPreProcessor(variableSetPreprocessor.Preprocess),


Preprocessor to switch Environment to VariableSet and DeprecatedAlias to pass the resourceType validations

xoscar · 2023-08-02T20:21:00Z

cli/cmd/resources.go

+	deprecatedEnvironmentClient = resourcemanager.NewClient(
+		httpClient, cliLogger,
+		"environment", "environments",
+		resourcemanager.WithProxyResource("variableset"),


Used to create an entry in the registry that will point to the variable set client

xoscar · 2023-08-02T20:21:31Z

cli/pkg/resourcemanager/registry.go

@@ -30,13 +31,33 @@ func (r *Registry) Get(resourceName string) (Client, error) {
 		return Client{}, ErrResourceNotFound
 	}

+	if c.options.proxyResource != "" {


If proxy, check if the proxied resource exists

xoscar · 2023-08-02T20:21:47Z

cli/pkg/resourcemanager/registry.go

-	for k := range r.resources {
-		resources = append(resources, k)
+	for k, c := range r.resources {
+		if c.options.proxyResource == "" {


Deleting the proxy resources from the list

xoscar · 2023-08-02T20:21:57Z

cli/preprocessor/variableset.go

+		return input, fmt.Errorf("could not unmarshal test yaml: %w", err)
+	}
+
+	if resource.Type == "Environment" {


Updates Environment to VariableSet

Does it make sense to add a //TODO comment saying that this mapping will be removed in future versions?

xoscar · 2023-08-02T20:22:22Z

testing/cli-e2etest/Makefile

@@ -17,6 +18,14 @@ test: # run tests for this application
 	go clean -testcache; \
 	go test -v -timeout 300s -p 1 ./...;

+test/scenario: # run tests for this application


This will allows us execute a single testscenario

Good idea! 🎉

xoscar · 2023-08-02T20:22:43Z

testing/cli-e2etest/testscenarios/test/run_test_test.go

@@ -85,20 +85,47 @@ func TestRunTestWithHttpTriggerAndEnvironmentFile(t *testing.T) {
 		require.Equal("https://assets.pokemon.com/assets/cms2/img/pokedex/full/143.png", environmentVars.Spec.Values[1].Value)
 	})

-	t.Run("should pass when using environment id", func(t *testing.T) {
+	t.Run("should pass when using the deprecated environment definition file", func(t *testing.T) {


Adding a test to validate that the old way still works as expected

xoscar · 2023-08-02T20:29:42Z

@danielbdias I added this test
https://github.com/kubeshop/tracetest/pull/3018/files#diff-5b0c58bde933c931dc8fbefd748fa0859adbb4fc222e7856ed2f35f4461b2898R88

To validate that the old way of using envs works as expected

danielbdias · 2023-08-02T21:17:32Z

cli/preprocessor/variableset.go

+		return input, fmt.Errorf("could not unmarshal test yaml: %w", err)
+	}
+
+	if resource.Type == "Environment" {


Does it make sense to add a //TODO comment saying that this mapping will be removed in future versions?

danielbdias · 2023-08-02T21:18:01Z

testing/cli-e2etest/Makefile

@@ -17,6 +18,14 @@ test: # run tests for this application
 	go clean -testcache; \
 	go test -v -timeout 300s -p 1 ./...;

+test/scenario: # run tests for this application


Good idea! 🎉

danielbdias · 2023-08-02T21:19:43Z

testing/cli-e2etest/testscenarios/variableset/apply_variableSet_test.go

nit: Can we name these files with smallcaps? (variableset instead of variableSet)

* chore(backend) Updating Environment to Variable Set (#3007) * chore(backend) Updating Environment to Variable Set * Updating open api * chore(frotend): Updating Environment to Variable Set (#3012) * chore(backend) Updating Environment to Variable Set * chore(frotend): Updating Environment to Variable Set * PR comments * chore(cli): Adding support for Variable Sets (#3018) * chore(cli): Adding support for Variable Sets * chore(cli): Fixing automated tests * chore(cli): Updating UI * adding environment backwards compability and tests * adding environment backwards compability and tests * PR review update

xoscar added 3 commits August 1, 2023 11:26

chore(cli): Adding support for Variable Sets

c0c0bca

chore(cli): Fixing automated tests

dd70fca

chore(cli): Updating UI

c716d7a

xoscar self-assigned this Aug 1, 2023

xoscar marked this pull request as ready for review August 1, 2023 18:52

xoscar requested review from schoren, mathnogueira, danielbdias and jorgeepc August 1, 2023 18:52

xoscar added the CLI label Aug 1, 2023

danielbdias reviewed Aug 2, 2023

View reviewed changes

xoscar added 2 commits August 2, 2023 14:18

adding environment backwards compability and tests

8ad39cc

adding environment backwards compability and tests

77010df

xoscar commented Aug 2, 2023

View reviewed changes

danielbdias approved these changes Aug 2, 2023

View reviewed changes

PR review update

7fc0ec2

xoscar merged commit 7dbc458 into 3000-rename-environments-to-variables Aug 3, 2023
29 of 30 checks passed

xoscar deleted the 3014-update-the-cli branch August 3, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cli): Adding support for Variable Sets #3018

chore(cli): Adding support for Variable Sets #3018

xoscar commented Aug 1, 2023 •

edited

Loading

danielbdias left a comment

danielbdias Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

xoscar Aug 2, 2023

danielbdias Aug 2, 2023

xoscar Aug 2, 2023

danielbdias Aug 2, 2023

xoscar Aug 2, 2023

xoscar commented Aug 2, 2023

danielbdias Aug 2, 2023

danielbdias Aug 2, 2023

danielbdias Aug 2, 2023

	runCmd.Flags().StringVarP(&runParams.VarsID, "vars", "", "", "variable set file or ID to be used")
	runCmd.Flags().StringVarP(&runParams.VarsID, "variableset", "", "", "variable set file or ID to be used")

chore(cli): Adding support for Variable Sets #3018

chore(cli): Adding support for Variable Sets #3018

Conversation

xoscar commented Aug 1, 2023 • edited Loading

Changes

Fixes

Checklist

danielbdias left a 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

xoscar commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoscar commented Aug 1, 2023 •

edited

Loading