🐛 Test and logic fixes #211
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/L
chuckha
force-pushed
the
more-test-cleanup
branch
from
b50e317 to
2786787
Compare
|
/assign @fabriziopandini @detiber |
| return ctrl.Result{}, err | ||
| } | ||
| if hashes != nil { | ||
| if config.Spec.JoinConfiguration.Discovery.BootstrapToken == nil { |
There was a problem hiding this comment.
IMO this piece of code should be moved into reconcileDiscovery , so all the logics impaction JoinConfiguration.Discovery are in the same place.
Nb. the bit of code for creating BootstrapToken if nil is already implemented there as well
There was a problem hiding this comment.
I don't think it makes sense to add this check to reconcileDiscovery unless you are also suggesting to add certificates into the reconcileDiscovery method. We look up certificates to see if we have a CACertHash to use. If we do then reconcileDiscovery does something different than if it didn't have a CACertHash. Therefore reconcileDiscovery depends on looking up certificates. Either certificates becomes part of reconcileDiscovery or we will have to live with not all defaults ending up in reconcileDiscovery
If we want to refactor it that's fine but I think that's out of scope for this PR.
There was a problem hiding this comment.
@chuckha thanks!
IMO we should move all the logic for defaulting JoinConfiguration.Discovery in the reconcileDiscovery func. Everything else looks great
chuckha
force-pushed
the
more-test-cleanup
branch
from
2786787 to
fde6452
Compare
chuckha
force-pushed
the
more-test-cleanup
branch
from
fde6452 to
8ad9c6e
Compare
Signed-off-by: Chuck Ha <[email protected]>
chuckha
force-pushed
the
more-test-cleanup
branch
from
8ad9c6e to
5d369d4
Compare
|
/lgtm |
Signed-off-by: Chuck Ha [email protected]
What this PR does / why we need it:
Some more test clean ups and the discovery of a bug where the user could not set UnsafeSkipCAVerification even if they wanted to. In addition, this adds error handling for a missed error check.