Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling #59

Open
shibumi opened this issue Jul 9, 2020 · 4 comments
Open

Error handling #59

shibumi opened this issue Jul 9, 2020 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@shibumi
Copy link
Collaborator

shibumi commented Jul 9, 2020

Please fill in the fields below to submit an issue or feature request. The
more information that is provided, the better.

Description of issue or feature request:
With Go 13, Go got better error handling with functions like errors.Is() or errors.As(). We might want to adapt our code, so developers can use this functions with our library.

Current behavior:

Right now we are excessively using fmt.Errorf("..") for error messages. Instead we should declare global Error types like in the Symlink implementation and return error types:

// ErrSymCycle signals a detected symlink cycle in our RecordArtifacts() function.
var ErrSymCycle = errors.New("symlink cycle detected")

This way we have error documentation and can use errors.Is(err, ErrSymCycle) for error checking. This is a huge benefit over string checking with if err.Error() == "..." { .. }, because with the latter we can run into a nil pointer dereference if Error() returns nil.

Expected behavior:

Use documented error types like:

// ErrSymCycle signals a detected symlink cycle in our RecordArtifacts() function.
var ErrSymCycle = errors.New("symlink cycle detected")

More about new Go 1.13 error handling can be found here: https://blog.golang.org/go1.13-errors

Feedback @SantiagoTorres @lukpueh ?

@SantiagoTorres
Copy link
Member

This sounds like a better, more idomatic way to do error handling. It also sounds it'd be super helpful to improve testing.

Is there a way to do this gradually?

@shibumi
Copy link
Collaborator Author

shibumi commented Jul 9, 2020

@SantiagoTorres Yes, I just pushed 3 new error types in my latest in-toto-run commit. The error handling gets so much better with this.

We get basically from something like this:

        expectedError := "Could not find a public key PEM block"
	err = key.LoadRSAPublicKey("demo.layout.template")
	if err == nil || !strings.Contains(err.Error(), expectedError) {
		t.Errorf("LoadRSAPublicKey returned (%s), expected '%s' error", err,
			expectedError)
	}

to:

        err = key.LoadRSAPublicKey("demo.layout.template")
	if !errors.Is(err, ErrNoPEMBlock) {
            t.Errorf("LoadRSAPublicKey returned (%s), expected '%s' error", err,
			ErrNoPEMBlock.Error())
	}

@lukpueh
Copy link
Member

lukpueh commented Jul 10, 2020

Cross-referencing with #5.

@shibumi shibumi added the enhancement New feature or request label Jan 15, 2021
@shibumi shibumi added the good first issue Good for newcomers label Apr 22, 2021
@sarthaksarthak9
Copy link

@shibumi is this issue still up ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants