servercfg.GetPublicIP fixes and unit tests #2616
Conversation
|
lmao this PR sent me into a whole adventure involving an open pull request to golang/go that has one positive code review so far https://go-review.googlesource.com/c/go/+/533119 |
abhishek9686
requested review from
Aceix,
theguy951357,
gabrielseibel1,
pwilson77,
abhishek9686 and
mattkasun
|
🤠 hold on there partner 🤠 @mattkasun this function is heavily used within the configuration package, not just that deprecated endpoint see servercfg |
my bad... mistakenly thought this PR was regarding controllers/getPublicIP() |
| if err != nil { | ||
| continue | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode == http.StatusOK { | ||
| bodyBytes, err := io.ReadAll(resp.Body) | ||
| var bodyBytes []byte |
There was a problem hiding this comment.
no need to declare a var here bodyBytes, err :=io.ReadAll(resp.Body)
There was a problem hiding this comment.
Well... this actually starts to touch on the bug that I'm fixing in the first place. The problem here is that we were checking for a non nil err afterwards that was... always nil... what I'm doing here is making sure that we are propagating any errors within the for loop to that err we declared in our encompassing scope.
Previously we were declaring a fresh err variable in our local scope, meaning that the original err was always going to be nil.
do note that yes of course that error may be replaced by another error within the for loop, but the way I see it is it's a hell of a lot better than just not knowing what happened (or even that it had an error at all).
In a function like this i figure as long as I get the value of one of the errors that's likely enough for me to troubleshoot the problem.
As long as the last IP endpoint we tried worked, we should get a nil error value and go about our business.
There was a problem hiding this comment.
the non-nil check after completing the loop is not required. It does not provide any additional information. Once the loop is completed, if the endpoint string is still empty, the error "public address not found" should be returned.
There was a problem hiding this comment.
okay, so wouldn't we like to know.. why? as my unit test shows, with this PR, we do (at least in regard to the error this function returns)
I'd like to know the reason that this function failed if it failed. even if it only shows me the last error received, chances are high that they were all the same error, and still much better than "He's dead, Jim." with no additional information.
Are you saying we're not handling the error from this function? if so then that'd be what I'll attack next.
There was a problem hiding this comment.
for the most part, this error is ignored by calling functions. In the one case where it is checked, an err will result in a fatalLog.
if we need the actual error for troubleshooting then just add a slog.Debug() when the error occurs.
There was a problem hiding this comment.
yeah that fatalLog is exactly what prompted this fix, because it was a fatality that gave no information.. sure tho
|
|
||
| // our unit test sets a bad ip service to test the timeout and fallback mechanics | ||
| // if that env variable is set, lower the timeout so the test doesn't take forever | ||
| testCheck := os.Getenv("NETMAKER_TEST_BAD_IP_SERVICE") |
There was a problem hiding this comment.
tests shouldn't part of the main code logic
Bing Bong! It's me, ya boi.
Describe your changes
Checklist before requesting a review
Other Notes
recommend using github's squash merge, but i can squash it for you if you'd like.