feat: list projects err handling by dbolson · Pull Request #21 · launchdarkly/ldcli

dbolson · 2024-03-13T18:28:59Z

Better error handling for missing flags and invalid responses.

You can see the results by running these commands

go run main.go projects
go run main.go projects list
go run main.go projects list --accessToken invalid
go run main.go projects list --accessToken invalid --baseUri invalid

…rr-handling

shortcut-integration · 2024-03-13T18:29:04Z

This pull request has been linked to Shortcut Story #235696: create command to list existing projects.

dbolson · 2024-03-13T18:33:33Z

 }

 func runList(cmd *cobra.Command, args []string) error {
-	// TODO: handle missing flags


These are handled in other places now.

dbolson · 2024-03-13T18:35:30Z

 	if err != nil {
-		os.Exit(1)
+		switch {
+		case errors.Is(err, errs.ErrUnauthorized):


This more closely matches the stripe CLI. If there's a structural error like a missing flag or invalid command, it returns an error and shows usage. If it's a logical error like an invalid access token or project key, it shows the error but not usage.

We can test this type of code once we have a higher level of dependency injection to pass in a mock API client from the highest level. Otherwise the tests will try to execute a real HTTP call.

Nice this makes sense/I like this

dbolson · 2024-03-13T18:36:09Z

+	err := rootCmd.MarkPersistentFlagRequired("accessToken")
 	if err != nil {
-		os.Exit(1)
+		panic(err)


These errors should never happen, but we should see what they are in case they do.

dbolson · 2024-03-13T18:37:11Z

@@ -0,0 +1,8 @@
+package errors


This package would shadow the standard library errors package, but the only place that should happen is at the highest level of the call stack. If you find you're creating errors.New then they should probably be defined in here instead.

dbolson · 2024-03-13T18:37:21Z

-func ListProjects(ctx context.Context, client2 Client) ([]byte, error) {
-	projects, err := client2.List(ctx)
+func ListProjects(ctx context.Context, client Client) ([]byte, error) {
+	projects, err := client.List(ctx)


dbolson · 2024-03-13T18:39:11Z

-		e, ok := err.(ldapi.GenericOpenAPIError)
-		if ok {
-			return e.Body(), err
+		if err.Error() == "401 Unauthorized" {


I couldn't set up a test with a GenericOpenAPIError that has specific data in it because its fields aren't exported. This seems simpler, at least for now.

dbolson · 2024-03-13T21:03:35Z

 		rootCmd.SetOut(actual)
 		rootCmd.SetErr(actual)
-		rootCmd.SetArgs([]string{"hello"})
+		rootCmd.SetArgs([]string{"hello", "--accessToken", "test"})


This is to get the tests to pass. I'll remove the hello world command in another PR.

dbolson · 2024-03-13T21:04:23Z

-	if viper.GetString("baseUri") == "" {
-		return errors.New("baseUri required")
+// validate ensures the flags are valid before using them.
+func validate(cmd *cobra.Command, args []string) error {


I'd prefer to have this in the root command so we don't need it for each command that uses the global flags, but we can figure that out later.

dbolson · 2024-03-13T21:06:21Z

 	err := rootCmd.Execute()
 	if err != nil {
-		os.Exit(1)
+		switch {


The idea here is to let errors bubble up and handle them in one spot. We can set whatever error we want in the subcommands to make this switch statement simpler to work with.

dbolson · 2024-03-13T21:06:54Z

 		"baseUri",
 		"u",
-		"http://localhost:3000",
+		"https://app.launchdarkly.com",


This changes the default to production so others aren't surprised when using the CLI.

dbolson · 2024-03-13T21:07:15Z

+		panic(err)
 	}

+	rootCmd.SetErrPrefix("")


This removes "Error: {msg}" and just shows the error message.

k3llymariee

one comment about validating the URI otherwise LGTM!

k3llymariee · 2024-03-13T22:07:51Z

 	if err != nil {
-		os.Exit(1)
+		switch {
+		case errors.Is(err, errs.ErrUnauthorized):


Nice this makes sense/I like this

k3llymariee · 2024-03-13T22:10:56Z

+	_, err := url.ParseRequestURI(viper.GetString("baseUri"))
+	if err != nil {
+		return errors.ErrInvalidBaseURI


should we also make sure that it's a valid LD URI?

I didn't want to do too much validation since it should only be an issue if someone is overriding the default, so they would be able to see the URL they put in. We could add more validation if we find people are confused.

Danny Olson added 12 commits March 11, 2024 10:01

WIP

f4c65bd

Merge branch 'main' into sc-235696/list-projects

6c25ca4

WIP: Call get projects and return as JSON

d5d9150

Merge branch 'main' into sc-235696/list-projects

6bc6f21

Don't need to expand when accessing V3 endpoint

4e3a1fd

Undo exported var

90fd9e8

Undo change

5657dbf

Add comment

c943717

WIP

b3347df

Use list subcommand for projects

2189e1c

Handle errors

3e0a218

Merge branch 'sc-235696/list-projects' into sc-235696/list-projects-e…

cfa1dcf

…rr-handling

Merge branch 'main' into sc-235696/list-projects-err-handling

a58f061

dbolson commented Mar 13, 2024

View reviewed changes

Validate baseUri flag

327369f

dbolson commented Mar 13, 2024

View reviewed changes

dbolson marked this pull request as ready for review March 13, 2024 21:07

Merge branch 'main' into sc-235696/list-projects-err-handling

f378d35

dbolson requested review from k3llymariee and sunnyguduru and removed request for sunnyguduru March 13, 2024 21:08

dbolson changed the title ~~sc-235696/list projects err handling~~ feat: list projects err handling Mar 13, 2024

Merge branch 'main' into sc-235696/list-projects-err-handling

cc9c53f

k3llymariee approved these changes Mar 13, 2024

View reviewed changes

Danny Olson added 2 commits March 13, 2024 15:14

Merge branch 'main' into sc-235696/list-projects-err-handling

a628860

Remove deleted file

6097aba

sunnyguduru approved these changes Mar 13, 2024

View reviewed changes

dbolson merged commit 2ab3668 into main Mar 13, 2024

dbolson deleted the sc-235696/list-projects-err-handling branch March 13, 2024 22:23

This was referenced Mar 13, 2024

chore(main): release 1.2.0 #26

Closed

chore(main): release 1.2.0 #32

Closed

github-actions bot mentioned this pull request Mar 20, 2024

chore(main): release 0.0.1 #46

Merged

github-actions bot mentioned this pull request Apr 8, 2024

chore(main): release 0.0.1 #144

Closed

This was referenced Apr 15, 2026

chore(main): release 3.0.0 #682

Closed

chore(main): release 0.12.1 #683

Closed

chore(main): release 0.12.1 #685

Closed

chore(main): release 0.12.1 #690

Closed

Conversation

dbolson commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shortcut-integration bot commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k3llymariee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dbolson commented Mar 13, 2024 •

edited

Loading