kubelet-config: fix variable shadowing #603
Conversation
Otherwise, we were only adding nil as Conditions even when there was an actual error... Signed-off-by: Antonio Murdaca <[email protected]>
openshift-ci-robot
added
approved
| if getErr != nil { | ||
| return getErr | ||
| } | ||
| newcfg.Status.Conditions = append(newcfg.Status.Conditions, wrapErrorWithCondition(err, args...)) |
There was a problem hiding this comment.
and maybe this needs to be wrapped in a if err != nil to avoid reporting nothing?
There was a problem hiding this comment.
wrapErrorWithCondition returns Success if error = nil (https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/kubelet-config/helpers.go#L119).
So to me it looks like it'll always append a new condition here and we don't need to avoid this step,
There was a problem hiding this comment.
ah but looks like the return below ends up making the syncHandler return nil on no-error runs. Maybe we want to report a success/no errors msg here?
|
looks like apiserver didn't come up in e2e /test e2e-aws |
|
I agree with @runcom that this function should be split and add that comments should be added to make this easier to understand at a glance. |
|
/lgtm I'll look into improving that status reporting. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rphillips, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6 participants
Otherwise, we were only adding nil as Conditions even when there was an
actual error...
@rphillips ptal, maybe this function should be split in 2 and unit tests added to catch stuff like this..
Signed-off-by: Antonio Murdaca [email protected]