adding nfd feature rule package and new builder #882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ggordaniRed
requested review from
wabouhamad,
kononovn,
trewest,
achuzhoy,
cdvultur and
klaskosk
as code owners
sebrandon1
reviewed
Jan 21, 2025
ggordaniRed
force-pushed
the
addingnfdrule
branch
from
a0ad42a to
28d8007
Compare
ggordaniRed
force-pushed
the
addingnfdrule
branch
from
28d8007 to
8f83e02
Compare
klaskosk
reviewed
Feb 5, 2025
| } | ||
|
|
||
| // GetErrorMessage returns builder's error message. | ||
| func (builder *NodeFeatureRuleBuilder) GetErrorMessage() string { |
There was a problem hiding this comment.
With the way the builders are designed, the error message should get returned when doing operations like creating, updating, or deleting. Is there a reason why you'd need to get the error message outside of that?
There was a problem hiding this comment.
could you please address this?
ggordaniRed
force-pushed
the
addingnfdrule
branch
from
8f83e02 to
dfa898c
Compare
klaskosk
reviewed
Feb 6, 2025
|
|
||
| return false, fmt.Errorf("%s builder cannot have nil apiClient", resourceCRD) | ||
| } | ||
|
|
There was a problem hiding this comment.
please add a check for errorMsg (example)
| // Created Builder object on the cluster. | ||
| Object *nfdv1.NodeFeatureRule | ||
| // api client to interact with the cluster. | ||
| apiClient *clients.Settings |
There was a problem hiding this comment.
another thing I missed, but this should be switched to the client from controller-runtime. We accept *clients.Settings in functions but only store apiClient.Client in the builder (example)
| } | ||
|
|
||
| // GetErrorMessage returns builder's error message. | ||
| func (builder *NodeFeatureRuleBuilder) GetErrorMessage() string { |
There was a problem hiding this comment.
could you please address this?
Comment on lines
+212
to
+222
| func buildTestClientWithNFDRuleScheme() *clients.Settings { | ||
| return clients.GetTestClients(clients.TestClientParams{ | ||
| SchemeAttachers: nfdRuleTestSchemes, | ||
| }) | ||
| } | ||
|
|
||
| func buildTestClientWithNFDRuleSchemeOnly() *clients.Settings { | ||
| return clients.GetTestClients(clients.TestClientParams{ | ||
| SchemeAttachers: nfdRuleTestSchemes, | ||
| }) | ||
| } |
Co-authored-by: Kirsten Laskoski <[email protected]>
Co-authored-by: Kirsten Laskoski <[email protected]>
Co-authored-by: Kirsten Laskoski <[email protected]>
Co-authored-by: Kirsten Laskoski <[email protected]>
Co-authored-by: Kirsten Laskoski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding new lib,builder,unit tests for nodfeaturerule object