Nodeselector Description update#1483
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions 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. |
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubevirt-bot
added
the
do-not-merge/release-note-label-needed
|
Hi @smulje. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the description of the NodeSelector field in the NodeNetworkConfigurationPolicy CRD to clarify its label matching behavior and limitations. However, the reviewer noted that these changes were made manually to the YAML file rather than the Go source code, which risks inconsistency and loss of updates during future manifest regeneration. It is recommended to apply these documentation improvements to the Go API definitions and use the generation tools to update the manifests.
| NodeSelector is a selector that determines which nodes the policy will be applied to. | ||
| It uses simple key-value label matching (equality-based selection only). All specified | ||
| labels must match a node's labels for the policy to be scheduled on that node. | ||
| Supports a maximum of 256 label selectors. Note: matchLabels and matchExpressions syntax | ||
| are not supported; only direct key-value pair matching is available. |
There was a problem hiding this comment.
This CRD file appears to have been updated manually, which presents two issues:
- Inconsistency: The corresponding manifest in
bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yamlwas not updated, leading to inconsistent documentation across the repository. - Maintainability: This file is typically generated from Go source code using
controller-gen(as indicated by the annotations in the bundle version). Manual changes will be lost the next time the manifests are regenerated.
Please apply these description updates to the docstrings of the NodeSelector field in the Go API definitions (likely in api/shared/ where NodeNetworkConfigurationPolicySpec is defined) and then run the manifest generation command (e.g., make manifests) to update all YAML files consistently.
Additionally, note a minor grammatical correction: "syntax are not supported" should be "syntax is not supported" or "syntaxes are not supported".
|
Kindly please use your corporate credentials for the commit sign off instead of the current |
smulje
force-pushed
the
NNCP-Nodeselector-Description-fix
branch
3 times, most recently
from
61f478a to
aafda55
Compare
| value: quay.io/nmstate/kubernetes-nmstate-handler:latest | ||
| - name: HANDLER_IMAGE_PULL_POLICY | ||
| value: IfNotPresent | ||
| value: Always |
| value: quay.io/openshift/origin-kube-rbac-proxy:4.10.0 | ||
| image: quay.io/nmstate/kubernetes-nmstate-operator:latest | ||
| imagePullPolicy: IfNotPresent | ||
| imagePullPolicy: Always |
smulje
force-pushed
the
NNCP-Nodeselector-Description-fix
branch
2 times, most recently
from
8678507 to
deb4170
Compare
This does not seem ready. I am pretty sure we wouldn't be happy merging the PR in its current state
|
Signed-off-by: Swati Mulje <smulje@redhat.com>
smulje
force-pushed
the
NNCP-Nodeselector-Description-fix
branch
from
deb4170 to
d0e178f
Compare
|
The suggested changes have been completed. Please review and let me know if everything looks good to proceed. |
3 participants
This PR updates the NodeSelector field description in the CRD to more accurately reflect its current behavior and limitations.
Previously, the description referenced general Kubernetes node selector semantics and linked to the upstream documentation, which may imply support for advanced selector features such as matchExpressions. However, the current implementation only supports simple equality-based key-value matching.
What’s changed
Clarified that NodeSelector uses simple key-value (equality-based) label matching only
Explicitly stated that all specified labels must match for the policy to be applied to a node
Added a clear note that matchLabels and matchExpressions are not supported
Why this change
The previous description could mislead users into assuming support for full Kubernetes label selector semantics. This update ensures the documentation is aligned with actual functionality, reducing confusion and preventing incorrect configurations.