Refactoring of image compatibility node validator #2028

mfranczy · 2025-01-23T13:27:58Z

I refactored the node validator code to achieve better readability for easier maintenance / expansion.
In addition, test cases have been moved into separate functions, before in case of failure, it was hard to figure out what's wrong.

k8s-ci-robot · 2025-01-23T13:28:08Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

netlify · 2025-01-23T13:28:17Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`99d63d3`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/67a088555f81dd0008d9587e
😎 Deploy Preview	https://deploy-preview-2028--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mfranczy · 2025-01-23T13:29:51Z

/hold

The PR is ready for review, but I want to be 110% sure I didn't break anything.. still running tests.

ChaoyiHuang · 2025-01-24T04:05:42Z

pkg/client-nfd/compat/node-validator/node-validator.go

+	for _, term := range featureMatcher {
+		if term.MatchExpressions != nil {
+			out = append(out, matchExpressions(term, matchedFeatureTerms)...)
+		}


continue to next loop if matchExpressions has been appended? not rely on if term.MatchExpressions != nil then term.MatchName must be nil

It's allowed to define matchExpressions and matchName at the same time. Both cases have to be checked before the next iteration.

pkg/client-nfd/compat/node-validator/node-validator.go

mfranczy · 2025-01-27T10:18:44Z

/hold cancel

The code works as expected.

marquiz · 2025-01-31T07:44:43Z

pkg/client-nfd/compat/node-validator/node-validator.go

+		if matchStatus.MatchAny[i].MatchedFeaturesTerms != nil {
+			matchedFeatureTermsAny = matchStatus.MatchAny[i].MatchedFeaturesTerms


nit: just caught my eye, small readability improvement (imo)

Suggested change

if matchStatus.MatchAny[i].MatchedFeaturesTerms != nil {

matchedFeatureTermsAny = matchStatus.MatchAny[i].MatchedFeaturesTerms

if t := matchStatus.MatchAny[i].MatchedFeaturesTerms; t != nil {

matchedFeatureTermsAny = t

marquiz · 2025-01-31T07:45:49Z

pkg/client-nfd/compat/node-validator/node-validator.go

+	if matchStatus.MatchFeatureStatus != nil {
+		matchedFeatureTerms = matchStatus.MatchFeatureStatus.MatchedFeaturesTerms
+	}


nit: just caught my eye, small readability improvement (imo)

Suggested change

if matchStatus.MatchFeatureStatus != nil {

matchedFeatureTerms = matchStatus.MatchFeatureStatus.MatchedFeaturesTerms

}

if m := matchStatus.MatchFeatureStatus; m != nil {

matchedFeatureTerms = m.MatchedFeaturesTerms

}

marquiz · 2025-01-31T07:51:17Z

pkg/client-nfd/compat/node-validator/node-validator_test.go

-			Version: compatv1alpha1.Version,
-			Compatibilties: []compatv1alpha1.Compatibility{
+
+		Convey("That contains flag which results in match", func() {


nit: with the new convey here, should the one above be changed to something like

Convey("With a single compatibility set", t func() {

Would it make more sense?

Yes, fixed.

marquiz · 2025-01-31T07:52:36Z

/ok-to-test

Additionally, testcases have been separated into distinct functions for better readability. Signed-off-by: Marcin Franczyk <[email protected]>

marquiz

Looks good to me
ping @ChaoyiHuang @ArangoGutierrez

k8s-ci-robot · 2025-02-03T14:35:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, mfranczy

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:

~~OWNERS~~ [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ChaoyiHuang · 2025-02-05T00:55:21Z

LGTM, sorry for the holiday break

marquiz · 2025-02-05T09:11:13Z

LGTM, sorry for the holiday break

NP, thanks @ChaoyiHuang
/lgtm

k8s-ci-robot · 2025-02-05T09:11:20Z

LGTM label has been added.

Git tree hash: 2bdfadf8010d8aa4f411fcabcad95a209362fc0e

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 23, 2025

k8s-ci-robot requested review from kad and TessaIO January 23, 2025 13:28

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 23, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 23, 2025

mfranczy force-pushed the image-compatibility-nfr branch from d7a5d3d to c402435 Compare January 23, 2025 13:31

ChaoyiHuang reviewed Jan 24, 2025

View reviewed changes

pkg/client-nfd/compat/node-validator/node-validator.go Outdated Show resolved Hide resolved

mfranczy force-pushed the image-compatibility-nfr branch from c402435 to b8c0924 Compare January 27, 2025 10:18

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2025

marquiz reviewed Jan 31, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 31, 2025

Refactoring of image compatibility node validator.

99d63d3

Additionally, testcases have been separated into distinct functions for better readability. Signed-off-by: Marcin Franczyk <[email protected]>

mfranczy force-pushed the image-compatibility-nfr branch from b8c0924 to 99d63d3 Compare February 3, 2025 09:11

marquiz approved these changes Feb 3, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2025

k8s-ci-robot assigned marquiz Feb 5, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2025

k8s-ci-robot merged commit 378d2ff into kubernetes-sigs:master Feb 5, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of image compatibility node validator #2028

Refactoring of image compatibility node validator #2028

mfranczy commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 •

edited

Loading

mfranczy commented Jan 23, 2025

ChaoyiHuang Jan 24, 2025

mfranczy Jan 24, 2025

mfranczy commented Jan 27, 2025

marquiz Jan 31, 2025

mfranczy Feb 3, 2025

marquiz Jan 31, 2025

mfranczy Feb 3, 2025

marquiz Jan 31, 2025

mfranczy Feb 3, 2025

marquiz commented Jan 31, 2025

marquiz left a comment

k8s-ci-robot commented Feb 3, 2025

ChaoyiHuang commented Feb 5, 2025

marquiz commented Feb 5, 2025

k8s-ci-robot commented Feb 5, 2025

		if matchStatus.MatchAny[i].MatchedFeaturesTerms != nil {
		matchedFeatureTermsAny = matchStatus.MatchAny[i].MatchedFeaturesTerms

Refactoring of image compatibility node validator #2028

Refactoring of image compatibility node validator #2028

Conversation

mfranczy commented Jan 23, 2025

k8s-ci-robot commented Jan 23, 2025

netlify bot commented Jan 23, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

mfranczy commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfranczy commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz commented Jan 31, 2025

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2025

ChaoyiHuang commented Feb 5, 2025

marquiz commented Feb 5, 2025

k8s-ci-robot commented Feb 5, 2025

netlify bot commented Jan 23, 2025 •

edited

Loading