feat(cli): refactor list formatter for better resource manager support #2829

schoren · 2023-06-26T23:28:44Z

This PR is a technical refactor that allows leveraging the server's resourcemanager formatting capabilities.

Currently, this PR only updates the LIST cli verb. Meaning that tracetest list [any resource] will use this new code. However, the rest of the verbs are unmodified. They'll be updated in following PRs.

The benefits of this refactor are

don't re implement features: the server already supports handling all the encodings and is very well tested.
avoids discrepancies generated by different encoding methods (server vs cli)

there is an extra benefit, that will only be visible once the refactor is completed, and that is, the amount of code needed to implement a resource (or maintain it, for that matter), is very close to zero. The only thing needed is this:

resources.Register(
	resourcemanager.NewClient(
		httpClient,
		"config", "configs",
		resourcemanager.TableConfig{
			Cells: []resourcemanager.TableCellConfig{
				{Header: "ID", Path: "spec.id"},
				{Header: "NAME", Path: "spec.name"},
				{Header: "ANALYTICS ENABLED", Path: "spec.analyticsEnabled"},
				// etc
			},
		},
	),
)

The rest is general to any resources, due to the standarization that resourcemanager provides.

Loom going over outpu
https://www.loom.com/share/73043af3a23e42e79255bed75e6df46c

Checklist

tested locally
added new dependencies
updated the docs
added a test

This reverts commit 74fa31c.

xoscar · 2023-06-27T14:56:13Z

@schoren do you have diagrams, plans or any information on how we want the CLI to work moving forward?

I really feel that if we go the path of having a "blind" version of the CLI which doesn't know about structures, types, etc might cause problems in the long run if we have to implement more than just a pass-through.

schoren · 2023-06-27T15:01:15Z

@xoscar what kind of problems do you see?

The whole point of having the resource manager was to have clients as thin as possible, and centralize all the encoding/decoding and other logics in the server/resource manager.

all resources behave exactly the same, as per the resource manager implementation.

Keep in mind that this way of handling things only applies for the CRUD operations, and does not exclude having more advanced implementations for other actions.

For example, the run verb does not follow under the resource manager umbrella, so any code the CLI needs for that can be added without interference from the resource manager code.

About diagrams, plans or any other information, we don't have that right now either. It doesn't feel like the current code had any architectural design behind it. Maybe if we could compare the current implementation design with the new proposed one we could get pros/cons of each.

xoscar · 2023-06-27T15:15:25Z

@schoren I can't think of a problem today, so if you feel this is the best approach and will serve any potential customization in the future that's fine.

schoren · 2023-06-27T23:42:13Z

testing/cli-e2etest/testscenarios/analyzer/list_analyzer_test.go

-		require.True(analyzerYAML[0].Spec.Enabled)
-		require.Equal(analyzerYAML[0].Spec.MinimumScore, 95)
-		require.Len(analyzerYAML[0].Spec.Plugins, 3)
+		analyzerList := helpers.UnmarshalJSON[types.ResourceList[types.AnalyzerResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

@schoren I think that's expected, the CLI is expected to return an array of resources:

--- resource1 --- resource2 --- resourc3

As per the requirements, you can see some of that from the CLI improvements epic

@xoscar The originally defined specs (environments for example #2182 (comment)) defined the json resposne to be a resource list. This allows direct passthrough from the server.

Keep in mind that count hold the total amount of available resources, not just the returned ones, so it's very useful for pagination. If you are using the JSON ouptut, it's reasonable to assume that this is a useful info to provide

So then, how should it work? ResourceList for json and plane array list for yaml?

@kdhamric had some comments about this first time we implement it as a ResourceList

The use case for each formmat are different.

The YAML list is almost surely used to export a list into a file to later import on another instace, hence the multi-document format.

JSON is mainly machine readable, so its most likely be used in scripting. In this case, you might need extra info.

for this same reason JSON uses the augmented verison and YAML doesn’t.

IIRC @kdhamric comment was around the yaml format and how that should be multidocument instead of a ResourceList. This is still the case here.

This change is only for the JSON format.

@xoscar @kdhamric I've added a loom about the output formats so we're all on the same page on what this PR proposes

Perfect, that makes sense, thanks for the video!

schoren · 2023-06-27T23:42:20Z

testing/cli-e2etest/testscenarios/config/list_config_test.go

-		require.Equal("current", configYAML[0].Spec.ID)
-		require.Equal("Config", configYAML[0].Spec.Name)
-		require.False(configYAML[0].Spec.AnalyticsEnabled)
+		configList := helpers.UnmarshalJSON[types.ResourceList[types.ConfigResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:42:24Z

testing/cli-e2etest/testscenarios/datastore/list_datastore_test.go

@@ -84,12 +84,13 @@ func TestListDatastore(t *testing.T) {
 		result := tracetestcli.Exec(t, "list datastore --sortBy name --output json", tracetestcli.WithCLIConfig(cliConfig))
 		helpers.RequireExitCodeEqual(t, result, 0)

-		dataStoresJSON := helpers.UnmarshalJSON[[]types.DataStoreResource](t, result.StdOut)
+		dataStoresList := helpers.UnmarshalJSON[types.ResourceList[types.DataStoreResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:42:28Z

testing/cli-e2etest/testscenarios/demo/list_demos_test.go

@@ -108,18 +108,19 @@ func TestListDemos(t *testing.T) {
 		result := tracetestcli.Exec(t, "list demo --sortBy name --sortDirection asc --output json", tracetestcli.WithCLIConfig(cliConfig))
 		helpers.RequireExitCodeEqual(t, result, 0)

-		demos := helpers.UnmarshalJSON[[]types.DemoResource](t, result.StdOut)
-		require.Len(demos, 2)
+		demos := helpers.UnmarshalJSON[types.ResourceList[types.DemoResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:42:36Z

testing/cli-e2etest/testscenarios/environment/list_environments_test.go

@@ -127,12 +127,13 @@ func TestListEnvironments(t *testing.T) {
 		result := tracetestcli.Exec(t, "list environment --sortBy name --sortDirection asc --output json", tracetestcli.WithCLIConfig(cliConfig))
 		helpers.RequireExitCodeEqual(t, result, 0)

-		environmentVarsList := helpers.UnmarshalJSON[[]types.EnvironmentResource](t, result.StdOut)
-		require.Len(environmentVarsList, 3)
+		environmentVarsList := helpers.UnmarshalJSON[types.ResourceList[types.EnvironmentResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:42:42Z

testing/cli-e2etest/testscenarios/pollingprofile/list_pollingprofile_test.go

-		require.Equal("periodic", pollingProfileYAML[0].Spec.Strategy)
-		require.Equal("50s", pollingProfileYAML[0].Spec.Periodic.RetryDelay)
-		require.Equal("10m", pollingProfileYAML[0].Spec.Periodic.Timeout)
+		pollingProfilesList := helpers.UnmarshalJSON[types.ResourceList[types.PollingProfileResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:42:46Z

testing/cli-e2etest/testscenarios/transaction/list_transactions_test.go

@@ -134,10 +134,11 @@ func TestListTransactions(t *testing.T) {
 		result := tracetestcli.Exec(t, "list transaction --sortBy name --sortDirection asc --output json", tracetestcli.WithCLIConfig(cliConfig))
 		helpers.RequireExitCodeEqual(t, result, 0)

-		transactions := helpers.UnmarshalJSON[[]types.AugmentedTransactionResource](t, result.StdOut)
-		require.Len(transactions, 3)
+		transactions := helpers.UnmarshalJSON[types.ResourceList[types.AugmentedTransactionResource]](t, result.StdOut)


this scenario was incorrect (as well as the old implementation). it was returning an array of resources instead of a ResourceList

schoren · 2023-06-27T23:44:59Z

cli/cmd/middleware.go

@@ -35,16 +34,19 @@ An error ocurred when executing the command
 	}
 }

-func WithParamsHandler(params ...parameters.Params) MiddlewareWrapper {
+func WithParamsHandler(validators ...Validator) MiddlewareWrapper {


this changes allows the resourcemanager client to provide its own parameters without needing to depend on the parameters package

schoren · 2023-06-27T23:47:02Z

cli/parameters/parameters.go

-	}
-
-	return errors
+func (pe paramError) Error() string {


in go, error is an interface, so implemeting this method makes the original paramError be treated as any other error. just this changes makes it compliant with the new validator approach from here

danielbdias

🤘

danielbdias · 2023-06-28T15:13:23Z

cli/cmd/config.go

+						if lastRunTime != "" {
+							date, err := time.Parse(time.RFC3339, lastRunTime)
+							if err != nil {
+								panic(err)


Should we panic here or only emit a warning log and set the time as empty?

good catch! I changed it to return an error!

* chore(docs): Adding App Insights Configuration Page (#2820) * chore(docs): Adding App Insights Configuration Page * fixing typo * fix(frontend): Fixing Resizable Panels UI bugs (#2827) * fix(tests): add analyzer resource to cli e2e table (#2826) * chore(docs): Azure App Insights Recipes (#2821) * chore(docs): Adding App Insights Configuration Page * chore(docs): Adding App Insights Recipes * chore(docs): Adding App Insights Recipes * chore(docs): fixing typo * adding recipe links * adding recipe links * fixing typo * chore(frotend): Test Definition Name Input (#2830) * chore(frotend): Updating Panel Splitter w/ Tooltip (#2828) * chore(frotend): Updating Panel Splitter w/ Tooltip * Updating theme color * removing unnecessary flag * undo change * fix(frontend): fix missing font-face (#2833) * feat(cli): refactor list formatter for better resource manager support (#2829) * feat(frontend): add independent trace vs test data (#2815) * feat(cli): refactor Get formatter for better resource manager support (#2831) * feat(cli): dynamic list of available resources (#2832) * feat(cli): refactor Delete to new resource manager client (#2836) * feat(cli): update export command with new resourcemanager client (#2838) * Standardize old resources to to use list method with SQL Injection protection and stardardize PollingProfile file (#2839) * wip: update list on demo and environment to use the same standards as tests and transactions * Fixing provisioning test * Update provisioning test * feature(frontend): Contact Us Modal (#2835) * feature(frontend): Contact Us Modal * feature(frontend): Contact Us Modal * feat(cli): update apply command with new resourcemanager client (#2841) * feat(frontend): trace vs test data for timeline and attribute list (#2837) * feat: new tests openapi spec (#2658) update get tests endpoint to use resource manager * fix: move transactions to it's own module (#2664) * fix: move transactions to the transactions folder * remove alias to transactions module * feat: test models + list tests endpoint (#2681) * add test, run, and trigger objects * feat: move test and run structs into the test package * list tests * add old format attributes again * fix mapping * compatibility mode * fix server prefix integration test * force server unit tests to run * fix tracetest test * trigger CI * Apply suggestions from code review Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * fix build * version test specs * update test names * use sqlutils * dont specify branch target in pull_request * fix list test --------- Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * 2659 cli improvements migrate tests to resource manager architecture get tests frontend (#2702) chore(frontend): updating FE to support GET /tests endpoint changes * feat: test list augmented response (#2732) add augmented list * fix(server): fix trigger json encoding versioning and test (#2795) * feat: test models + list tests endpoint (#2681) * add test, run, and trigger objects * feat: move test and run structs into the test package * list tests * add old format attributes again * fix mapping * compatibility mode * fix server prefix integration test * force server unit tests to run * fix tracetest test * trigger CI * Apply suggestions from code review Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * fix build * version test specs * update test names * use sqlutils * dont specify branch target in pull_request * fix list test --------- Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * GET endpoint * fix rebase * add create method and fix tests * add rest of methods * Changing test structure and fixing method by method * Update tests * Fixing test errors for Delete * Adding gRPC tests * Adding tests for traceid trigger * migrate rest of endpoints and fix compilation errors * fix all unit and integration tests * Improving tests * fix building errors --------- Co-authored-by: Oscar Reyes <oscar-rreyes1@hotmail.com> Co-authored-by: Jorge Padilla <jorge.esteban.padilla@gmail.com> Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> Co-authored-by: Daniel Dias <danielbpdias@gmail.com>

* feat: new tests openapi spec (#2658) update get tests endpoint to use resource manager * fix: move transactions to it's own module (#2664) * fix: move transactions to the transactions folder * remove alias to transactions module * feat: test models + list tests endpoint (#2681) * add test, run, and trigger objects * feat: move test and run structs into the test package * list tests * add old format attributes again * fix mapping * compatibility mode * fix server prefix integration test * force server unit tests to run * fix tracetest test * trigger CI * Apply suggestions from code review Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * fix build * version test specs * update test names * use sqlutils * dont specify branch target in pull_request * fix list test --------- Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * 2659 cli improvements migrate tests to resource manager architecture get tests frontend (#2702) chore(frontend): updating FE to support GET /tests endpoint changes * feat: test list augmented response (#2732) add augmented list * fix(server): fix trigger json encoding versioning and test (#2795) * WIP feat: get endpoint (#2789) * chore(docs): Adding App Insights Configuration Page (#2820) * chore(docs): Adding App Insights Configuration Page * fixing typo * fix(frontend): Fixing Resizable Panels UI bugs (#2827) * fix(tests): add analyzer resource to cli e2e table (#2826) * chore(docs): Azure App Insights Recipes (#2821) * chore(docs): Adding App Insights Configuration Page * chore(docs): Adding App Insights Recipes * chore(docs): Adding App Insights Recipes * chore(docs): fixing typo * adding recipe links * adding recipe links * fixing typo * chore(frotend): Test Definition Name Input (#2830) * chore(frotend): Updating Panel Splitter w/ Tooltip (#2828) * chore(frotend): Updating Panel Splitter w/ Tooltip * Updating theme color * removing unnecessary flag * undo change * fix(frontend): fix missing font-face (#2833) * feat(cli): refactor list formatter for better resource manager support (#2829) * feat(frontend): add independent trace vs test data (#2815) * feat(cli): refactor Get formatter for better resource manager support (#2831) * feat(cli): dynamic list of available resources (#2832) * feat(cli): refactor Delete to new resource manager client (#2836) * feat(cli): update export command with new resourcemanager client (#2838) * Standardize old resources to to use list method with SQL Injection protection and stardardize PollingProfile file (#2839) * wip: update list on demo and environment to use the same standards as tests and transactions * Fixing provisioning test * Update provisioning test * feature(frontend): Contact Us Modal (#2835) * feature(frontend): Contact Us Modal * feature(frontend): Contact Us Modal * feat(cli): update apply command with new resourcemanager client (#2841) * feat(frontend): trace vs test data for timeline and attribute list (#2837) * feat: new tests openapi spec (#2658) update get tests endpoint to use resource manager * fix: move transactions to it's own module (#2664) * fix: move transactions to the transactions folder * remove alias to transactions module * feat: test models + list tests endpoint (#2681) * add test, run, and trigger objects * feat: move test and run structs into the test package * list tests * add old format attributes again * fix mapping * compatibility mode * fix server prefix integration test * force server unit tests to run * fix tracetest test * trigger CI * Apply suggestions from code review Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * fix build * version test specs * update test names * use sqlutils * dont specify branch target in pull_request * fix list test --------- Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * 2659 cli improvements migrate tests to resource manager architecture get tests frontend (#2702) chore(frontend): updating FE to support GET /tests endpoint changes * feat: test list augmented response (#2732) add augmented list * fix(server): fix trigger json encoding versioning and test (#2795) * feat: test models + list tests endpoint (#2681) * add test, run, and trigger objects * feat: move test and run structs into the test package * list tests * add old format attributes again * fix mapping * compatibility mode * fix server prefix integration test * force server unit tests to run * fix tracetest test * trigger CI * Apply suggestions from code review Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * fix build * version test specs * update test names * use sqlutils * dont specify branch target in pull_request * fix list test --------- Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> * GET endpoint * fix rebase * add create method and fix tests * add rest of methods * Changing test structure and fixing method by method * Update tests * Fixing test errors for Delete * Adding gRPC tests * Adding tests for traceid trigger * migrate rest of endpoints and fix compilation errors * fix all unit and integration tests * Improving tests * fix building errors --------- Co-authored-by: Oscar Reyes <oscar-rreyes1@hotmail.com> Co-authored-by: Jorge Padilla <jorge.esteban.padilla@gmail.com> Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> Co-authored-by: Daniel Dias <danielbpdias@gmail.com> * fix a couple of issues * fix wrong variable name * fix part of the tracetests * Fixing small bugs on server * Fixing test run problem * fix: tracetests (#2852) fix grpc tests and transaction tracetests * fixing FE tests and types * Updating test assertion validation * Tests cli resourcemanager client (#2849) * fixing FE tests and updating models * fixing FE tests and updating models * Updating trigger mapping to populate httpRequest field on OpenAPI based endpoints * Removing deprecated fields * fix trigger type in fe * add cli e2e test for test resource (#2854) * Fixed bug in dogfooding test * Making backend-arch-graph step not block CI on failure * fix trigger result type in fe * Deprecate commands (#2857) * Move test deprecated notice to subcommands * Update CLI e2e docs * fix(docs): fix deprecated test command (#2874) --------- Co-authored-by: Matheus Nogueira <matheus.nogueira2008@gmail.com> Co-authored-by: Sebastian Choren <schoren@users.noreply.github.com> Co-authored-by: Jorge Padilla <jorge.esteban.padilla@gmail.com> Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com> Co-authored-by: Daniel Dias <danielbpdias@gmail.com> Co-authored-by: Sebastian Choren <sebastian.choren@gmail.com>

schoren added 12 commits June 26, 2023 14:30

implement list client and basic formatters

6eb6205

finish list implementation

3627f59

add new field in server

74fa31c

normalize validation to go error interface

2da4229

add missing default headers

fe8f497

fix server bug

7a8990b

enable new list for all resources

3f01bf6

add option to modify row

94c5feb

Revert "add new field in server"

50df52f

This reverts commit 74fa31c.

add missing transaction field

7206443

add comments

58fb014

fix default format setting

8720f8a

xoscar approved these changes Jun 27, 2023

View reviewed changes

schoren added 6 commits June 27, 2023 12:15

fix yamlstream bug

02386e1

add pagination params to request

62fe082

handle more encoding cases

d75076d

fix tests

a587631

hide zero dates

6eea27f

fix bug

0ae3076

schoren commented Jun 27, 2023

View reviewed changes

schoren requested review from mathnogueira and danielbdias June 27, 2023 23:55

schoren mentioned this pull request Jun 28, 2023

feat(cli): refactor Get formatter for better resource manager support #2831

Merged

4 tasks

danielbdias approved these changes Jun 28, 2023

View reviewed changes

jorgeepc approved these changes Jun 28, 2023

View reviewed changes

improve ItemModifier error handling

280a19a

schoren merged commit e6a6d5e into main Jun 28, 2023
25 checks passed

schoren deleted the cli-format-from-server branch June 28, 2023 18:44

This was referenced Jun 28, 2023

feat(cli): refactor Delete to new resource manager client #2836

Merged

feat(cli): update export command with new resourcemanager client #2838

Merged

feat(cli): update apply command with new resourcemanager client #2841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): refactor list formatter for better resource manager support #2829

feat(cli): refactor list formatter for better resource manager support #2829

schoren commented Jun 26, 2023 •

edited

xoscar commented Jun 27, 2023

schoren commented Jun 27, 2023 •

edited

xoscar commented Jun 27, 2023

schoren Jun 27, 2023

xoscar Jun 28, 2023

schoren Jun 28, 2023 •

edited

xoscar Jun 28, 2023

xoscar Jun 28, 2023

schoren Jun 28, 2023

schoren Jun 28, 2023

schoren Jun 28, 2023 •

edited

xoscar Jun 28, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

schoren Jun 27, 2023

danielbdias left a comment

danielbdias Jun 28, 2023

schoren Jun 28, 2023

feat(cli): refactor list formatter for better resource manager support #2829

feat(cli): refactor list formatter for better resource manager support #2829

Conversation

schoren commented Jun 26, 2023 • edited

Checklist

xoscar commented Jun 27, 2023

schoren commented Jun 27, 2023 • edited

xoscar commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schoren Jun 28, 2023 • edited

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

schoren Jun 28, 2023 • edited

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

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

schoren commented Jun 26, 2023 •

edited

schoren commented Jun 27, 2023 •

edited

schoren Jun 28, 2023 •

edited

schoren Jun 28, 2023 •

edited