Validate Node Name #48
Conversation
razo7
commented
May 10, 2023
•
razo7
commented
- Validate FAR CR name to a node name in the cluster.
- Add three new verbs to check whether a specific node name match the nodes' names in the cluster.
- Add and update unit-tests
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 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 |
razo7
force-pushed
the
validate-node-name
branch
2 times, most recently
from
b73169c
to
b81611f
Compare
We want to validate that CR has been created correctly. Also UT functionality
We need to add three new rbac, get, list and watch, for listing the cluster nodes
| return emptyResult, err | ||
| } | ||
| r.Log.Error(err, "Failed to validate FAR CR's name- retry") | ||
| return ctrl.Result{RequeueAfter: apiFailureRetryTimeout}, nil |
There was a problem hiding this comment.
Why not just return the error here ?
(returning an error will trigger another reconcile)
razo7
force-pushed
the
validate-node-name
branch
2 times, most recently
from
da7cdee
to
6e1b89d
Compare
pkg/utils/nodes.go
Outdated
| // IsNodeNameValid returns an error if nodeName doesn't match any node name int the cluster, otherwise a nil | ||
| func IsNodeNameValid(r client.Reader, nodeName string) (bool, error) { | ||
| node := &corev1.Node{} | ||
| nodeExist := true |
There was a problem hiding this comment.
what do you need this for? IMHO it would be easier to read when using true or false directly
if isNotFound(err) {
return false, nil
} else if err != nil {
return false, err
}
return true, nil
or if you want to keep error handling nested:
if err != nil {
if isNotFound(err) {
return false, nil
}
return false, err
}
return true, nil
There was a problem hiding this comment.
I though it looks nicer and I like variables :) .
But I guess there is no actual benefit of using variables instead of explicitly the bool value here (of course in this situation).
There was a problem hiding this comment.
I went the second option, since I am not sure whether isNotFound would accept checking the input err when it is nil
Delete redundant error file, and returning bool with error for IsNodeNameValid function
|
/lgtm |
| @@ -69,39 +72,53 @@ var _ = Describe("FAR Controller", func() { | |||
| underTestFAR = newFenceAgentsRemediation(validNodeName, fenceAgentIPMI, testShareParam, testNodeParam) | |||
|
|
|||
| Context("Functionality", func() { | |||
| Context("buildFenceAgentParams", func() { | |||
| When("FAR's name isn't a node name", func() { | |||
| Context("buildFenceAgentParams - check CR name", func() { | |||
There was a problem hiding this comment.
Nit: using function names in Ginkgo's node description makes it harder to understand what the test is verifying.
For example, in this context, it might be just "Check CR name against node"
There was a problem hiding this comment.
Ok, do you have a suggestion for better naming?
I see a small value of testing the 2 functions in UT, but it does catch errors when I do small changes in them (or their dependencies)
There was a problem hiding this comment.
For example, in this context, it might be just "Check CR name against node"
There was a problem hiding this comment.
I see, then I tend to let the context just be the function name (buildFenceAgentParams), and just remove the - check CR name
There was a problem hiding this comment.
not super important, but this would be less future-proof IMHO
If you change function name, or signature, you shall then change the test too
Instead, testing a behavior, whatever the implementation is, should always be valid also in the future
EDIT: I removed "change signature", which cannot be future proof, ofc XD
There was a problem hiding this comment.
Indeed, checking behavior rather than a function (by it's name) seems more right.
Something to think off for next PRs and writing UTs
Updating import names, and check error message of 1 UT
|
/lgtm giving others the chance to check if their reviews are addressed |
|
/lgtm giving others the chance to check if their reviews are addressed |
|
I believe 2/3 acks is good enough for this PR 🙂 /unhold |
The E2E test is redundant after medik8s#48 PR, and it also fails here when we try to add/remove taint for a node which doesn't exist - dummy-node
The E2E test is redundant after medik8s#48 PR, and it also fails here when we try to add/remove taint for a node which doesn't exist - dummy-node
The E2E test is redundant after medik8s#48 PR, and it also fails here when we try to add/remove taint for a node which doesn't exist - dummy-node
The E2E test is redundant after medik8s#48 PR, and it also fails here when we try to add/remove taint for a node which doesn't exist - dummy-node
