-
Notifications
You must be signed in to change notification settings - Fork 238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
source/custom: add internal rule api #1479
source/custom: add internal rule api #1479
Conversation
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
8b17971
to
5cbab6d
Compare
Let's wait for #1480 at least |
11553e6
to
17b2184
Compare
17b2184
to
6599bcc
Compare
Rebased |
} | ||
|
||
// Validate validates the expression. | ||
func (m *MatchExpression) Validate() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we leverage the new Validate PKG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not for this internal API.
But that could be possible further change/enhancement on top of this PR. It'd be a slightly bigger change than initially thought.
Add internal API for the nfd-worker custom feature source rule configuration. This API has already diverged from the NFD NodeFeatureRule API in that annotations, extended resources or taints are not supported. This patch basically copies the Rule type (and it's sub-types) from the nfdv1alpha1 package. It also adds conversion functions from the internal rule API to the "external" nfdv1alpha1 API. This is done to use the same rule matching functionality (from the nfdv1alpha1 package). One notable remark is that the feature source rule config supports some custom formatting (short forms, multi-type fields) that relies on special json/yaml unmarshalling functions that are better to nuke from the nfdv1alpha1 package (in another patch). These (legacy) syntax specialities are most probably used by nobody but let's keep them as they're already there. Unit tests to cover the custom json unmarshalling are now added.
Change the custom feature source of nfd-worker to use the newly added internal config API for its own configuration. It now uses the internal types for json/yaml unmarshalling but converts them to external nfdv1alpha1 API to do the actual rule matching as the internal API does not duplicate that functionality.
Not needed in the external API.
6599bcc
to
b28d5c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
package api | ||
|
||
// Rule defines a rule for node customization such as labeling. | ||
type Rule struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like having duplicated cope like this. but for now it's ok.
I think for v0.16 I want to work on moving Rule type outside the v1alpha1 folder/pkg to clean the api pkg and also to have all that functionality in a more centralized location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding duplication, I cannot think of a better alternative. The two APIs (the CRD API and the custom source configuration) have diverged. In terms of typedefs the custom source config is a subset of the CRD API, but in terms of "syntax", it's a superset as it supports these int-or-string-or-bool kind of fields and some historical shortforms etc
We cannot move the typedef outside the nfdv1alpha1 package as that's part of the API (or we technically could import it from anywhere but I think that's a bad idea because that other package then essentially becomes a part of the API...). For the "functionality", i.e. the implementation of Rule evaluation/processing, I agree that we want to move it out of the API package.
LGTM label has been added. Git tree hash: be44725c918f2f505100fec2d50b837e807443b3
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArangoGutierrez, marquiz 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 |
/test pull-node-feature-discovery-build-image-cross-generic |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1479 +/- ##
==========================================
+ Coverage 31.27% 31.94% +0.67%
==========================================
Files 59 61 +2
Lines 7649 7722 +73
==========================================
+ Hits 2392 2467 +75
+ Misses 5009 5004 -5
- Partials 248 251 +3
|
Add new internal "API" for the
Add internal API for the nfd-worker custom feature source rule
configuration. This API has already diverged from the NFD
NodeFeatureRule API in that annotations, extended resources or taints
are not supported. This patch basically copies the Rule type (and it's
sub-types) from the nfdv1alpha1 package. It also adds conversion
functions from the internal rule API to the "external" nfdv1alpha1 API.
This is done to use the same rule matching functionality (from the
nfdv1alpha1 package).
One notable remark is that the feature source rule config supports some
custom formatting (short forms, multi-type fields) that relies on
special json/yaml unmarshalling functions that are better to nuke from
the nfdv1alpha1 package (in another patch). These (legacy) syntax
specialities are most probably used by nobody but let's keep them as
they're already there. Unit tests to cover the custom json
unmarshalling are now added.
Use the new internal api for nfd-worker config parsing
Change the custom feature source of nfd-worker to use the newly added
internal config API for its own configuration. It now uses the internal
types for json/yaml unmarshalling but converts them to external
nfdv1alpha1 API to do the actual rule matching as the internal API does
not duplicate that functionality.
Drop custom unmarshaller functions from the nfdv1alpha1 API