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

controller check error #599

Merged
merged 2 commits into from
May 30, 2019

Conversation

subpathdev
Copy link
Member

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
In this line we can see, that the CommTo return nil or an error. https://github.com/kubeedge/kubeedge/blob/master/edge/pkg/devicetwin/dtcontext/dtcontext.go#L71

In this PR I take this error and create an error in function distributeMsg (

func (dtc *DTController) distributeMsg(m interface{}) error {
). This will be catched here: (
err := dtc.distributeMsg(msg)
)

In my opinion we should check if an error occur or not when it is possible. This will also increase the possibility to debug the program.

Which issue(s) this PR fixes:

Special notes for your reviewer:

@kubeedge-bot kubeedge-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 29, 2019
edge/pkg/devicetwin/dtcontroller.go Outdated Show resolved Hide resolved
edge/pkg/devicetwin/dtcontroller.go Outdated Show resolved Hide resolved
edge/pkg/devicetwin/dtcontroller.go Outdated Show resolved Hide resolved
edge/pkg/devicetwin/dtcontroller.go Outdated Show resolved Hide resolved
@m1093782566
Copy link

Squash commits please.

@subpathdev subpathdev force-pushed the dtControllerCheckError branch from e2c1348 to ee0633b Compare May 29, 2019 07:06
@subpathdev
Copy link
Member Author

/retest

@m1093782566
Copy link

/lgtm

leave approve to @sids-b @edisonxiang

@kubeedge-bot kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2019
@subpathdev
Copy link
Member Author

I didn't understand why the build failed. (Here is a success travis build on the same changes: https://travis-ci.org/subpathdev/kubeedge )

@m1093782566
Copy link

/retest

sids-b
sids-b previously approved these changes May 29, 2019
Copy link
Member

@sids-b sids-b left a comment

Choose a reason for hiding this comment

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

LGTM

@sids-b
Copy link
Member

sids-b commented May 29, 2019

@subpathdev , you need to make changes in the UT's as this err return would not be there in the code and it will continue to execute

@sids-b
Copy link
Member

sids-b commented May 29, 2019

The overall change looks good to me though

@sids-b sids-b dismissed their stale review May 29, 2019 08:32

Need to fix UT

@@ -330,6 +330,24 @@ func TestDTController_distributeMsg(t *testing.T) {
}
})
}

//Sucess Case

Choose a reason for hiding this comment

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

nit: s/Sucess/Successful

@sids-b
Copy link
Member

sids-b commented May 30, 2019

LGTM Overall. Can merge this PR once @m1093782566 review comments fixed

@m1093782566
Copy link

@subpathdev

Please squash the commits.

* create test case distributeMsg chan no found
* create test case distributeMsg sucessfull
@subpathdev subpathdev force-pushed the dtControllerCheckError branch from c4b1a00 to 336014d Compare May 30, 2019 07:48
@m1093782566
Copy link

/approve

THANKS!

@kubeedge-bot kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2019
@kubeedge-bot kubeedge-bot merged commit 0ddc4b3 into kubeedge:master May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants