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

Use k8s apimachinery for label validation #219

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Feb 20, 2019

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 20, 2019
@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 Feb 20, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 20, 2019
main.go Outdated
@@ -376,6 +376,10 @@ func getFeatureLabels(source source.FeatureSource) (labels Labels, err error) {

// Validate label value
value := fmt.Sprintf("%v", v)
if len(value) > 63 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would declare constant like in pkg/apis/core/validation/validation.go, so it will be understandable where this 63 value came from.

const qualifiedNameMaxLength int = 63

Otherwise, lgtm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually why not use validation from apimachinery directly.

I'll get back to this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this patch to use apimachinery directly for all validation

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 21, 2019
@marquiz marquiz changed the title Validate length of label values Use k8s apimachinery for label validation Feb 21, 2019
@marquiz
Copy link
Contributor Author

marquiz commented Feb 21, 2019

@kad ping, I rewrote the patch

@kad
Copy link
Contributor

kad commented Feb 21, 2019

lgtm

@marquiz marquiz merged commit 91b0714 into kubernetes-sigs:master Feb 22, 2019
@marquiz marquiz deleted the devel/label-verify branch February 22, 2019 07:45
@marquiz marquiz mentioned this pull request May 9, 2019
10 tasks
ArangoGutierrez pushed a commit to ArangoGutierrez/node-feature-discovery that referenced this pull request Nov 18, 2020
Version 0.4.0

Node-feature-discovery was migrated into a new repository under the
kubernetes-sigs organization in Github (kubernetes-sigs#175). Related to the migration,
the final container image registry/repo hasn't been dediced yet (kubernetes-sigs#177) –
for this release we still use the old repo.

Major changes
- Split NFD into client and server (kubernetes-sigs#209)
- Changes in labels:
  - NFD label namespace was changed to 'feature.node.kubernetes.io/' (kubernetes-sigs#176)
    - 'nfd-' prefix was dropped from all feature labels
    - NFD version label
      (feature.node.kubernetes.io/node-feature-discovery.version) was
      replaced by an annotation (nfd.node.kubernetes.io/version)
  - network SRIOV labels were changed (kubernetes-sigs#173):
    - 'network-sriov' -> 'network-sriov.capable'
    - 'network-sriov-configured' -> 'network-sriov.configured'
  - selinux detection was moved to kernel feature source
    - 'selinux' -> 'kernel-selinux.enabled'
  - cpuid, pstate and RDT labels moved under cpu feature source (kubernetes-sigs#217)
    - 'cpuid-<cpuid flag>' -> 'cpu-cpuid.<cpuid flag>'
    - 'pstate-turbo' -> 'cpu-pstate.turbo'
    - 'rdt-<rdt feature>' -> 'cpu-rdt.<rdt feature>'
- Support for config file (kubernetes-sigs#169). Currently with three configurable
  feature sources i.e. cpu (kubernetes-sigs#224), kernel (kubernetes-sigs#157) and pci (kubernetes-sigs#168)
- Support for non-binary labels, with arbitrary values other than plain
  'true'
- PCI device detection (kubernetes-sigs#168)
- Kernel version detection (kubernetes-sigs#157)
- Kernel config option detection (kubernetes-sigs#146)
- Support for custom feature-detector hooks (kubernetes-sigs#144)
- Support OS version detection (kubernetes-sigs#149, kubernetes-sigs#211)
- Detection of hardware multithreading, such as Intel Hyper-Threading
  Technology (kubernetes-sigs#147)
- Arm64 support for CPUID detection (kubernetes-sigs#194)
- Validation of feature label names and values (kubernetes-sigs#199, kubernetes-sigs#219)
- Detection of NVDIMM devices (kubernetes-sigs#214)
- Get labels by reading from file in 'local' source (kubernetes-sigs#228)
- Detection of Intel SST-BF (Speed Select Technology - Base Frequency) (kubernetes-sigs#235)
- Make it possible to create feature labels in non-default namespace
  (kubernetes-sigs#231). Currently possible for using the local source (hooks and files).

Miscellaneous
- Template specs converted from json to yaml
- Documentation updates and fixes
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants