Repository: kubernetes-sigs/node-feature-discovery
Pull request #1477 (2023-11-29 13:41:54)

apis/nfd: drop the private template caching fields
Drop the private fields – that were supposed to be used for caching parsed templates – from the Rule type. Keep the API typedefs cleaner and simpler. Moreover, the caching was not even used in practice, effectively complicating code without any benefit: the way the types are used in nfd-master creates a local copy of Rule type storing the cached template in the copy, wasting it from any future users.

There are also other possible caveats in caching like we tried to do it. For example the objects returned by the api lister are supposed to be treated as read-only - in particular if we would be to modify them there should at least be proper locking in place as nfd-master potentially processes the same rule (the same Go object) in parallel for multiple nodes. If any optimization like this will be pursued it should be done properly, probably with private type(s) at the consumer's end, not contaminating the API types.

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *kubernetes-sigs-nfd* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | fca8ee83b6e750065be6035cc50c41debf4e3ca6 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/65673fa5f22890000878d02f |
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/1477#" title="Author self-approved">marquiz</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fnode-feature-discovery).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/OWNERS)~~ [marquiz]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
> Merging [#1477](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (fca8ee8) into [master](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/b087c602c0e159b4676a9bee541364de68e41ecf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (b087c60) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   30.36%   30.40%   +0.03%     
==========================================
  Files          58       58              
  Lines        7508     7486      -22     
==========================================
- Hits         2280     2276       -4     
+ Misses       4976     4960      -16     
+ Partials      252      250       -2     
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/v1alpha1/rule.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3J1bGUuZ28=) | `83.97% <100.00%> (+4.56%)` | :arrow_up: |
| [pkg/apis/nfd/v1alpha1/zz\_generated.deepcopy.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3p6X2dlbmVyYXRlZC5kZWVwY29weS5nbw==) | `26.68% <ø> (+0.58%)` | :arrow_up: |

</details>
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
> Merging [#1477](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (fca8ee8) into [master](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/b087c602c0e159b4676a9bee541364de68e41ecf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (b087c60) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   30.36%   30.40%   +0.03%     
==========================================
  Files          58       58              
  Lines        7508     7486      -22     
==========================================
- Hits         2280     2276       -4     
+ Misses       4976     4960      -16     
+ Partials      252      250       -2     
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/v1alpha1/rule.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3J1bGUuZ28=) | `83.97% <100.00%> (+4.56%)` | :arrow_up: |
| [pkg/apis/nfd/v1alpha1/zz\_generated.deepcopy.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3p6X2dlbmVyYXRlZC5kZWVwY29weS5nbw==) | `26.68% <ø> (+0.58%)` | :arrow_up: |

</details>
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
> Merging [#1477](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (fca8ee8) into [master](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/ea1264ae6830b8f67212dc25cd0f2003ac8bee0a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (ea1264a) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head fca8ee8 differs from pull request most recent head dfc8bf2. Consider uploading reports for the commit dfc8bf2 to get more accurate results

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   30.36%   30.40%   +0.03%     
==========================================
  Files          58       58              
  Lines        7508     7486      -22     
==========================================
- Hits         2280     2276       -4     
+ Misses       4976     4960      -16     
+ Partials      252      250       -2     
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/v1alpha1/rule.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3J1bGUuZ28=) | `83.97% <100.00%> (+4.56%)` | :arrow_up: |
| [pkg/apis/nfd/v1alpha1/zz\_generated.deepcopy.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3p6X2dlbmVyYXRlZC5kZWVwY29weS5nbw==) | `26.68% <ø> (+0.58%)` | :arrow_up: |

</details>
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
> Merging [#1477](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (dfc8bf2) into [master](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/ea1264ae6830b8f67212dc25cd0f2003ac8bee0a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) (ea1264a) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   30.36%   30.40%   +0.03%     
==========================================
  Files          58       58              
  Lines        7508     7486      -22     
==========================================
- Hits         2280     2276       -4     
+ Misses       4976     4960      -16     
+ Partials      252      250       -2     
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/apis/nfd/v1alpha1/rule.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3J1bGUuZ28=) | `83.97% <100.00%> (+4.56%)` | :arrow_up: |
| [pkg/apis/nfd/v1alpha1/zz\_generated.deepcopy.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1477?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL2FwaXMvbmZkL3YxYWxwaGExL3p6X2dlbmVyYXRlZC5kZWVwY29weS5nbw==) | `26.68% <ø> (+0.58%)` | :arrow_up: |

</details>
marquiz: /assign @ArangoGutierrez 

Patch:
diff --git a/pkg/apis/nfd/v1alpha1/rule.go b/pkg/apis/nfd/v1alpha1/rule.go
index 7aeac42d8..2eb78dd87 100644
--- a/pkg/apis/nfd/v1alpha1/rule.go
+++ b/pkg/apis/nfd/v1alpha1/rule.go
@@ -112,15 +112,12 @@ func (r *Rule) executeLabelsTemplate(in matchedFeatures, out map[string]string)
 		return nil
 	}
 
-	if r.labelsTemplate == nil {
-		t, err := newTemplateHelper(r.LabelsTemplate)
-		if err != nil {
-			return fmt.Errorf("failed to parse LabelsTemplate: %w", err)
-		}
-		r.labelsTemplate = t
+	th, err := newTemplateHelper(r.LabelsTemplate)
+	if err != nil {
+		return fmt.Errorf("failed to parse LabelsTemplate: %w", err)
 	}
 
-	labels, err := r.labelsTemplate.expandMap(in)
+	labels, err := th.expandMap(in)
 	if err != nil {
 		return fmt.Errorf("failed to expand LabelsTemplate: %w", err)
 	}
@@ -134,15 +131,13 @@ func (r *Rule) executeVarsTemplate(in matchedFeatures, out map[string]string) er
 	if r.VarsTemplate == "" {
 		return nil
 	}
-	if r.varsTemplate == nil {
-		t, err := newTemplateHelper(r.VarsTemplate)
-		if err != nil {
-			return err
-		}
-		r.varsTemplate = t
+
+	th, err := newTemplateHelper(r.VarsTemplate)
+	if err != nil {
+		return err
 	}
 
-	vars, err := r.varsTemplate.expandMap(in)
+	vars, err := th.expandMap(in)
 	if err != nil {
 		return err
 	}
@@ -216,22 +211,6 @@ func newTemplateHelper(name string) (*templateHelper, error) {
 	return &templateHelper{template: tmpl}, nil
 }
 
-// DeepCopy is a stub to augment the auto-generated code
-func (h *templateHelper) DeepCopy() *templateHelper {
-	if h == nil {
-		return nil
-	}
-	out := new(templateHelper)
-	h.DeepCopyInto(out)
-	return out
-}
-
-// DeepCopyInto is a stub to augment the auto-generated code
-func (h *templateHelper) DeepCopyInto(out *templateHelper) {
-	// HACK: just re-use the template
-	out.template = h.template
-}
-
 func (h *templateHelper) execute(data interface{}) (string, error) {
 	var tmp bytes.Buffer
 	if err := h.template.Execute(&tmp, data); err != nil {
diff --git a/pkg/apis/nfd/v1alpha1/types.go b/pkg/apis/nfd/v1alpha1/types.go
index a9c8bcbcb..8934b6482 100644
--- a/pkg/apis/nfd/v1alpha1/types.go
+++ b/pkg/apis/nfd/v1alpha1/types.go
@@ -178,10 +178,6 @@ type Rule struct {
 	// MatchAny specifies a list of matchers one of which must match.
 	// +optional
 	MatchAny []MatchAnyElem `json:"matchAny"`
-
-	// private helpers/cache for handling golang templates
-	labelsTemplate *templateHelper `json:"-"`
-	varsTemplate   *templateHelper `json:"-"`
 }
 
 // MatchAnyElem specifies one sub-matcher of MatchAny.
diff --git a/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
index 60335468c..2d4cdbca9 100644
--- a/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/apis/nfd/v1alpha1/zz_generated.deepcopy.go
@@ -503,14 +503,6 @@ func (in *Rule) DeepCopyInto(out *Rule) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
-	if in.labelsTemplate != nil {
-		in, out := &in.labelsTemplate, &out.labelsTemplate
-		*out = (*in).DeepCopy()
-	}
-	if in.varsTemplate != nil {
-		in, out := &in.varsTemplate, &out.varsTemplate
-		*out = (*in).DeepCopy()
-	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Rule.


Test patch:
diff --git a/pkg/apis/nfd/v1alpha1/rule_test.go b/pkg/apis/nfd/v1alpha1/rule_test.go
index 8608df99a..16d45eff4 100644
--- a/pkg/apis/nfd/v1alpha1/rule_test.go
+++ b/pkg/apis/nfd/v1alpha1/rule_test.go
@@ -369,17 +369,14 @@ var-2=
 	assert.Equal(t, map[string]string{"foo": "bar"}, m.Labels, "instances should have matched")
 	assert.Empty(t, m.Vars)
 
-	r2.labelsTemplate = nil
 	r2.LabelsTemplate = "foo"
 	_, err = r2.Execute(f)
 	assert.Error(t, err)
 
-	r2.labelsTemplate = nil
 	r2.LabelsTemplate = "{{"
 	_, err = r2.Execute(f)
 	assert.Error(t, err)
 
-	r2.labelsTemplate = nil
 	r2.LabelsTemplate = ""
 	r2.VarsTemplate = "bar=baz"
 	m, err = r2.Execute(f)
@@ -387,12 +384,10 @@ var-2=
 	assert.Empty(t, m.Labels)
 	assert.Equal(t, map[string]string{"bar": "baz"}, m.Vars, "instances should have matched")
 
-	r2.varsTemplate = nil
 	r2.VarsTemplate = "bar"
 	_, err = r2.Execute(f)
 	assert.Error(t, err)
 
-	r2.varsTemplate = nil
 	r2.VarsTemplate = "{{"
 	_, err = r2.Execute(f)
 	assert.Error(t, err)