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

bugfix: remove duplicate error msgs #1048

Merged
merged 1 commit into from Apr 3, 2018

Conversation

faycheng
Copy link
Contributor

@faycheng faycheng commented Apr 3, 2018

Signed-off-by: 程飞 fay.cheng.cn@gmail.com

Ⅰ. Describe what this PR did

pouch daemon will print duplicate error messages if fails to create endpoint.
so, i remove duplicate output in func EndpointCreate.

ERRO[2018-04-03 20:40:33.310004469] failed to create endpoint, err: service endpoint with name 169a941d already exists
ERRO[2018-04-03 20:40:33.310011239] failed to create endpoint: service endpoint with name 169a941d already exists

Thanks a lot for your reviews and comments.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Apr 3, 2018
@@ -209,14 +208,12 @@ func (nm *NetworkManager) EndpointCreate(ctx context.Context, endpoint *types.En
if sb == nil {
sandboxOptions, err := nm.sandboxOptions(endpoint)
if err != nil {
logrus.Errorf("failed to build sandbox options, err: %v", err)
return "", err
return "", fmt.Errorf("failed to build sandbox options, err: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This removal LGTM, while I wish we could do a little more change:

how about
fmt.Errorf("failed to build sandbox options: %v", err) rather than
fmt.Errorf("failed to build sandbox options, err: %v", err)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@allencloud
Copy link
Collaborator

allencloud commented Apr 3, 2018

The log is already showed in https://github.com/alibaba/pouch/blob/master/daemon/mgr/container.go#L502

if _, err := mgr.NetworkMgr.EndpointCreate(ctx, endpoint); err != nil {
	logrus.Errorf("failed to create endpoint: %v", err)
	return err
}

So I think the removal is reasonable.
cc @rudyfly WDYT?

Signed-off-by: 程飞 <fay.cheng.cn@gmail.com>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Apr 3, 2018
@allencloud allencloud merged commit 1a614a6 into AliyunContainerService:master Apr 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants