feat(cli): dynamic list of available resources #2832

schoren · 2023-06-28T14:12:15Z

This PR updates the setup of commands using the new resourcemanager client to use a dynamically generated list of available (registered) resources for the help message. This removes the need to manually maintain a separated list.

This reverts commit 74fa31c.

danielbdias · 2023-06-28T18:29:58Z

cli/cmd/config.go

+		hc := resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)
+		*httpClient = *hc


Question: is there a reason to do this pointer trick instead of doing something like this:

Suggested change

hc := resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)

*httpClient = *hc

httpClient = &resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)

Yes. In order to have this dynamic list available, we need to register all resources before the commands are registered. To do this, the resources are registered as a package variable here, and the commands are registered within init functions. This guarantees that the registry will be ready before the commands.

The problem with this is that the resources depened on the resourcemanager.HTTPClient, which in turns depends on the config being ready. The config gets ready in the setupCommand function, which is called as a preRun of the commands. So in order to configure a command, you need to have already configured the command. This is a ciruclar reference initialization, and that's not allowed.

The solution I found was to setup the registry with a pointer to an empty HTTPClient, and when the setupCommand runs on the preRun stage of the command, it replace the pointer with the correctly configured client.

Does this make sense?

Yes, thanks!

Just one detail: can we add a small comment on the code explaining that? Maybe just a link to you explanation on the PR

Good idea. Added!

danielbdias · 2023-06-28T18:31:35Z

cli/cmd/get_cmd.go

+		Long:    "Get a resource from your Tracetest server",
+		PreRun:  setupCommand(),
+		Run: WithResourceMiddleware(func(_ *cobra.Command, args []string) (string, error) {
+			resourceType := args[0]


Can we get a panic here if the user runs the command just as tracetest get ?
Should we validate the args size and print the help on this case?

WithResourceMiddleware validates that the resource is set before getting to this function. Here's an example of running list without passing a resource:

It makes sense. My fear about accessing indexes in languages that throw out-of-index errors and friends is that sooner or later, we can receive that error (usually on dev mode when we coded something wrong by mistake xD).

One thing that we can think about in the future is that since the WithResourceMiddleware is tailored to the resources API, we could unpack the first argument and change the interface to func(_ *cobra.Command, resourceType string, args []string) (string, error) and allow args to have only the args for that command in fact.

sooner or later, we can receive that error
This is true. Assuming that the middleware will always work perfectly might not be the safest approach. This is actually being parsed to the resourceParams.ResourceName property, so I replaced the calls to args[0] to resourceParams.ResourceName.

What do you think @danielbdias? It doesn't guarantee success, but at least it doesn't panic

It sounds good to me!

danielbdias · 2023-06-28T18:32:51Z

cli/cmd/list_cmd.go

+			resourceType := args[0]
+			ctx := context.Background()


Same question as I did on get. Can we have a panic here if the user executes only tracetest list?

same answer as on get. this ges validated on the middleware before getting here

* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Jorge Padilla <[email protected]> Co-authored-by: Sebastian Choren <[email protected]> Co-authored-by: Daniel Baptista Dias <[email protected]> Co-authored-by: Daniel Dias <[email protected]>

* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Jorge Padilla <[email protected]> Co-authored-by: Sebastian Choren <[email protected]> Co-authored-by: Daniel Baptista Dias <[email protected]> Co-authored-by: Daniel Dias <[email protected]> * 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 <[email protected]> Co-authored-by: Sebastian Choren <[email protected]> Co-authored-by: Jorge Padilla <[email protected]> Co-authored-by: Daniel Baptista Dias <[email protected]> Co-authored-by: Daniel Dias <[email protected]> Co-authored-by: Sebastian Choren <[email protected]>

schoren added 22 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

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

initial implementation

eda0195

make default format configurable per verb

1d72bf8

fix 404 handling

e5f8c61

feat(cli): dynamic list of available resources

7e2a891

schoren requested review from mathnogueira and danielbdias June 28, 2023 14:12

fix httpclient setting

f112ab7

jorgeepc approved these changes Jun 28, 2023

View reviewed changes

danielbdias approved these changes Jun 28, 2023

View reviewed changes

schoren force-pushed the cli-format-get branch from df9d7d5 to cc6870c Compare June 28, 2023 18:51

Base automatically changed from cli-format-get to main June 28, 2023 19:09

Merge branch 'main' into remove-hardcoded-resource-list

4556a55

schoren added 4 commits June 28, 2023 16:16

fix merge error

9571dbc

replace args direct access with resourceParams

63de52a

add comment

8805312

add comment

aeb8ed0

schoren merged commit b72bb20 into main Jun 28, 2023

schoren deleted the remove-hardcoded-resource-list branch June 28, 2023 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): dynamic list of available resources #2832

feat(cli): dynamic list of available resources #2832

schoren commented Jun 28, 2023

danielbdias Jun 28, 2023

schoren Jun 28, 2023

danielbdias Jun 28, 2023

danielbdias Jun 28, 2023

schoren Jun 28, 2023

danielbdias Jun 28, 2023

schoren Jun 28, 2023

danielbdias Jun 28, 2023

schoren Jun 28, 2023

danielbdias Jun 28, 2023

danielbdias Jun 28, 2023

schoren Jun 28, 2023

		hc := resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)
		httpClient = hc

	hc := resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)
	httpClient = hc
	httpClient = &resourcemanager.NewHTTPClient(cliConfig.URL(), extraHeaders)

feat(cli): dynamic list of available resources #2832

feat(cli): dynamic list of available resources #2832

Conversation

schoren commented Jun 28, 2023

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