Lokomotive: Update error wrapping to new Golang conventions

Created on 27 Feb 2020  路  3Comments  路  Source: kinvolk/lokomotive

Since Go 1.13 there's a new way to work with errors. We should go through our use of fmt.Errorf and errors.Wrap* to make sure we are following it.

low-hanging-fruit

Most helpful comment

While we're touching errors, I think we should properly format error strings:

  • Error strings should not be capitalized (unless it's the beginning of the error string, i.e. the place where the error gets printed).
  • Error strings should not end in a period.

I'm also not fond of things like "failed to do this and that" (again, unless it's the beginning of the error string). Since error strings are typically chained, we can skip the "failed/couldn't/error" text and just give context:

:heavy_check_mark: fmt.Errorf("rendering manifest: %v", err)
:x: fmt.Errorf("error rendering manifest: %v", err)
:x: fmt.Errorf("rendering manifest failed: %v", err)
:x: fmt.Errorf("could not render manifest: %v", err)
:x: fmt.Errorf("I really wanted to render this manifest but alas, I have failed: %v", err)

Following the above, the user can get a useful error:

Error deploying cluster: deploying control plane: rendering manifest: file 'stuff' doesn't exist

All 3 comments

I think eventually, we should completely get rid of github.com/pkg/errors as a depedency.

388 addresses that.

While we're touching errors, I think we should properly format error strings:

  • Error strings should not be capitalized (unless it's the beginning of the error string, i.e. the place where the error gets printed).
  • Error strings should not end in a period.

I'm also not fond of things like "failed to do this and that" (again, unless it's the beginning of the error string). Since error strings are typically chained, we can skip the "failed/couldn't/error" text and just give context:

:heavy_check_mark: fmt.Errorf("rendering manifest: %v", err)
:x: fmt.Errorf("error rendering manifest: %v", err)
:x: fmt.Errorf("rendering manifest failed: %v", err)
:x: fmt.Errorf("could not render manifest: %v", err)
:x: fmt.Errorf("I really wanted to render this manifest but alas, I have failed: %v", err)

Following the above, the user can get a useful error:

Error deploying cluster: deploying control plane: rendering manifest: file 'stuff' doesn't exist

Was this page helpful?
0 / 5 - 0 ratings