Skip to content
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

pkg/apis/nfd: stricter format checking for template labels #668

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Nov 24, 2021

Require that the expanded LabelsTemplate has values. That is, the
(expanded) template must consist of key=value pairs separated by
newlines. No default value will be assigned and we now return an error
if a (non-empty) line not conforming with the key=value format is
encountered.

Commit c8d7366 described that the value defaults to "true" if not
specified. That was not the case and we defaulted to an empty string,
instead.

An example:

  - name: "my rule"
    labelsTemplate: |
      my.label.1=foo
      my.label.2=

Would create these labels:

  "my.label.1": "foo"
  "my.label.2": ""

Further, the following:

  - name: "my failing rule"
    labelsTemplate: |
      my.label.3

will cause an error in the rule processing.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 24, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 24, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 24, 2021

An alternative to solution would be to ditch the default value ("true") and e.g. just error out if the value part is missing (i.e. the expanded template value does not have an equals sign). I'm fine with that solution, too, if people like it more. The reasoning behind the default "true" value is that it aligns with the format of hooks and feature files

@marquiz
Copy link
Contributor Author

marquiz commented Nov 24, 2021

Will change this
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2021
@marquiz marquiz force-pushed the fixes/rule-template branch from fa853ae to a8057b1 Compare November 24, 2021 19:12
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 24, 2021
@marquiz marquiz changed the title pkg/apis/nfd: use "true" as the default value for template labels pkg/apis/nfd: stricter format checking for template labels Nov 24, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Nov 24, 2021

An alternative to solution would be to ditch the default value ("true") and e.g. just error out if the value part is missing (i.e. the expanded template value does not have an equals sign).

Changed the PR to behave like this.

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2021
@marquiz marquiz force-pushed the fixes/rule-template branch from a8057b1 to 9758ece Compare November 24, 2021 19:20
Require that the expanded LabelsTemplate has values. That is, the
(expanded) template must consist of key=value pairs separated by
newlines. No default value will be assigned and we now return an error
if a (non-empty) line not conforming with the key=value format is
encountered.

Commit c8d7366 described that the value defaults to "true" if not
specified. That was not the case and we defaulted to an empty string,
instead.

An example:

  - name: "my rule"
    labelsTemplate: |
      my.label.1=foo
      my.label.2=

Would create these labels:

  "my.label.1": "foo"
  "my.label.2": ""

Further, the following:

  - name: "my failing rule"
    labelsTemplate: |
      my.label.3

will cause an error in the rule processing.
@marquiz marquiz force-pushed the fixes/rule-template branch from 9758ece to 8a4d316 Compare November 24, 2021 19:31
@zvonkok
Copy link
Contributor

zvonkok commented Nov 25, 2021

We can make the API more restrictive and loosen up when needed, the other way around could be problematic.
Like this way much better.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 25, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0b4050a into kubernetes-sigs:master Nov 25, 2021
@marquiz marquiz deleted the fixes/rule-template branch November 25, 2021 10:49
@marquiz marquiz mentioned this pull request Dec 22, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants