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

Stop wrapping and returning nil #245

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Stop wrapping and returning nil #245

merged 1 commit into from
Jan 9, 2019

Conversation

nak3
Copy link
Contributor

@nak3 nak3 commented Nov 27, 2018

Description of changes:

Currently err in arg of the second errors.Wrapf() in getENIaddresses() will never
receive error except for nil value.

  • L#584 checks err != nil and return, hence L#595 will never get err except for nil.
    ec2Addrs, _, err := c.awsClient.DescribeENI(eni)
    if err != nil {
    return nil, "", errors.Wrapf(err, "fail to find eni addresses for eni %s", eni)
    }
    for _, ec2Addr := range ec2Addrs {
    if aws.BoolValue(ec2Addr.Primary) {
    eniPrimaryIP := aws.StringValue(ec2Addr.PrivateIpAddress)
    return ec2Addrs, eniPrimaryIP, nil
    }
    }
    return nil, "", errors.Wrapf(err, "faind to find eni's primary address for eni %s", eni)

When errors.Wrapf() is called with nil in first arg, the new message
is not wrapped. To make matters worse, getENIaddresses() returns
error as nil so the logic of code after the method could be collapsed.

Hence, this patch uses errors.Errorf() instead errors.Wrapf().

Also, current unit tests for ipamd use two primary IPs for one
ENI. This patch fixes it as well.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nak3
Copy link
Contributor Author

nak3 commented Nov 27, 2018

Here is a small test code when errors.Wrap gets nil.

package main

import (
	"fmt"
	"github.com/pkg/errors"
)

func main() {
	var err error

	err = nil
	err = errors.Wrap(err, "I want to check this error")

	fmt.Printf("error w/ nil:  %v\n", err)
	if err == nil {
		fmt.Printf("error is still nil.\n")
	}

	/* OUTPUT
	$ go run a.go
	error w/ nil:  <nil>
	error is still nil.
	*/
}

Currently second `errors.Wrapf()` in getENIaddresses() will never
receive error except for nil value.

When `errors.Wrapf()` is called with nil in first arg, the new message
is not wrapped. To make matters worse, `getENIaddresses()` returns
error as nil so the logic of code after the method could be collapsed.

Hence, this patch uses `errors.Errorf()` instead `errors.Wrapf()`.
Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM.

@mogren mogren merged commit 2209485 into aws:master Jan 9, 2019
@nak3 nak3 deleted the nil-error branch January 9, 2019 23:51
@mogren mogren added this to the v1.4 milestone Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants