-
Notifications
You must be signed in to change notification settings - Fork 245
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
Introduce nfd client for image compatibilty #1932
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mfranczy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @mfranczy. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
b269c77
to
86ddc6a
Compare
/ok-to-test |
86ddc6a
to
e05aa41
Compare
1097251
to
1d6d96a
Compare
7491e5a
to
6227de7
Compare
@marquiz PTAL |
d91a178
to
125f36c
Compare
125f36c
to
9bfe920
Compare
/retest |
The current rules implementation doesn’t easily match executed features (whether failed or succeeded) with expressions in the spec, as it was not designed for that. Without changes,
A run strategy implementation is required because, by default, rule processing stops at the first failure. |
20dc841
to
c1c214d
Compare
After discussions with end users, it became clear that the minimal functionality wasn't sufficient, so I revisited the problem from a new perspective. Previously, I attempted to handle expression execution status similar to MatchedFeatures, which introduced significant complexity and required extensive changes to the NFR code. I found a better approach by extending the expression struct with an isMatch field, allowing the expression status to update directly during processing. @ArangoGutierrez @marquiz apologies for the multiple updates, but this is the final version. The PR is ready for review, and I consider it complete. Please review whenever you have a moment. This commit shows the changes to NFR code. |
/retest |
3346daf
to
502e89e
Compare
/retest |
The strategy pattern allows for either executing all rules or failing fast. For image compatibility, it’s necessary to always execute all rules to identify which ones fail. Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
502e89e
to
cf7b0ba
Compare
/retest |
The PR provides an initial implementation of #1845. Image compatibility is defined by Node Feature Rules, so a change was also necessary there. A run strategy pattern has been implemented to control rule execution, allowing either all rules to be executed or stopping at the first failure.
The work is still in progress but is ready for the first review round.Remaining tasks:- [x] write unit tests- [x] write documentation and add appropriate code comments- [x] refine the commands