Parameterize inclusion of "remediation.medik8s.io/include-in-remediation" label for nodeAffinity#286
Conversation
…ion" label in DaemonSet nodeAffinity configuration.
📝 WalkthroughWalkthroughThe changes introduce a new optional boolean configuration field Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/selfnoderemediationconfig_types.go`:
- Around line 117-121: Fix the grammar in the comment for the
EnableIncludeInRemediationLabel field: update the phrase "as a required for the
pod to be deployed on specific node." to a grammatically correct form such as
"as required for the pod to be deployed on a specific node." Ensure you edit the
comment immediately above the EnableIncludeInRemediationLabel bool declaration
in selfnoderemediationconfig_types.go to use the corrected wording.
In
`@config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml`:
- Around line 122-127: The CRD YAML has a field ordering mismatch: move the
field named enableIncludeInRemediationLabel so its key appears alphabetically
before endpointHealthCheckUrl (i.e., regenerate rather than manually
reordering). Run the code-generator to produce a correct manifest: re-run
controller-gen (or run the project's make manifests target) to regenerate
config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml
so that enableIncludeInRemediationLabel is placed in proper alphabetical order
alongside isSoftwareRebootEnabled and endpointHealthCheckUrl, then commit the
regenerated file.
In `@install/self-node-remediation-deamonset.yaml`:
- Around line 38-43: This block is a Go template so the yaml linter
false-positive should be suppressed and the indentation normalized: ensure the
conditional block starting with "{{ if .EnableIncludeInRemediationLabel }}" and
ending with "{{ end }}" remains as-is (it's valid in templates), but align the
list item under "values:" so "- \"true\"" uses the same column alignment as the
existing similar expression (make the dash start directly under the first
character of "values:"'s line for consistency); additionally add a file-level
linter exception or suppress rule for Go template files so the yamlint warning
is ignored for this template.
- Around line 38-43: Add unit tests in
selfnoderemediationconfig_controller_test.go that exercise syncConfigDaemonSet
for both values of EnableIncludeInRemediationLabel: create one test case with
EnableIncludeInRemediationLabel = true and assert the rendered DaemonSet
contains the nodeAffinity/nodeSelectorTerm for
remediation.medik8s.io/include-in-remediation (operator In, value "true"), and
another with EnableIncludeInRemediationLabel = false and assert that
affinity/label selector is absent; instantiate the config used by
syncConfigDaemonSet, call the function to render the DaemonSet, and use
assertions on the returned podSpec/affinity and label selector fields to
validate presence/absence.
---
Duplicate comments:
In
`@bundle/manifests/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml`:
- Around line 124-129: The field enableIncludeInRemediationLabel is out of
alphabetical order in the CRD manifest (it was manually edited); revert this
manual change and regenerate the CRD YAML from the CRD source so fields are
emitted in canonical order (instead of editing enableIncludeInRemediationLabel
in the generated manifest). Locate the schema for the SelfNodeRemediationConfig/
selfnoderemediationconfigs CRD and run the project’s CRD generation command (or
rebuild the manifest generator) so enableIncludeInRemediationLabel appears in
the correct alphabetical position in the generated YAML.
| // EnableIncludeInRemediationLabel indicates whether we will include remediation.medik8s.io/include-in-remediation | ||
| // label in nodeAffinity configuration as a required for the pod to be deployed on specific node. | ||
| // +kubebuilder:default=false | ||
| // +optional | ||
| EnableIncludeInRemediationLabel bool `json:"enableIncludeInRemediationLabel,omitempty"` |
There was a problem hiding this comment.
Nit: grammatical error in doc comment.
"as a required for the pod" should be "as required for the pod" (or "as a requirement for the pod").
✏️ Proposed fix
-// EnableIncludeInRemediationLabel indicates whether we will include remediation.medik8s.io/include-in-remediation
-// label in nodeAffinity configuration as a required for the pod to be deployed on specific node.
+// EnableIncludeInRemediationLabel indicates whether we will include the remediation.medik8s.io/include-in-remediation
+// label in nodeAffinity configuration as required for the pod to be deployed on a specific node.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // EnableIncludeInRemediationLabel indicates whether we will include remediation.medik8s.io/include-in-remediation | |
| // label in nodeAffinity configuration as a required for the pod to be deployed on specific node. | |
| // +kubebuilder:default=false | |
| // +optional | |
| EnableIncludeInRemediationLabel bool `json:"enableIncludeInRemediationLabel,omitempty"` | |
| // EnableIncludeInRemediationLabel indicates whether we will include the remediation.medik8s.io/include-in-remediation | |
| // label in nodeAffinity configuration as required for the pod to be deployed on a specific node. | |
| // +kubebuilder:default=false | |
| // +optional | |
| EnableIncludeInRemediationLabel bool `json:"enableIncludeInRemediationLabel,omitempty"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v1alpha1/selfnoderemediationconfig_types.go` around lines 117 - 121, Fix
the grammar in the comment for the EnableIncludeInRemediationLabel field: update
the phrase "as a required for the pod to be deployed on specific node." to a
grammatically correct form such as "as required for the pod to be deployed on a
specific node." Ensure you edit the comment immediately above the
EnableIncludeInRemediationLabel bool declaration in
selfnoderemediationconfig_types.go to use the corrected wording.
| enableIncludeInRemediationLabel: | ||
| default: false | ||
| description: |- | ||
| Enable inclusion of "remediation.medik8s.io/include-in-remediation" label in nodeAffinity configuration | ||
| for DaemonSet as a required for the pod to be deployed on specific node. | ||
| type: boolean |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Check if file is auto-generated and look for Makefile targets
head -50 config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml | grep -i "controller-gen\|generate"
echo "---"
grep -n "manifests\|controller-gen" Makefile | head -10Repository: medik8s/self-node-remediation
Length of output: 1047
🏁 Script executed:
# Extract all property names from the spec section to verify ordering
rg -A 200 "properties:" config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml | rg "^\s{14}\w+:" | head -30Repository: medik8s/self-node-remediation
Length of output: 611
Field is out of alphabetical order — regenerate rather than manually edit.
enableIncludeInRemediationLabel should appear before endpointHealthCheckUrl (alphabetically, 'a' < 'd'), not after isSoftwareRebootEnabled. This file is auto-generated by controller-gen (v0.20.0), and manual edits diverge from the make manifests target. Regenerate to restore proper alphabetical ordering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml`
around lines 122 - 127, The CRD YAML has a field ordering mismatch: move the
field named enableIncludeInRemediationLabel so its key appears alphabetically
before endpointHealthCheckUrl (i.e., regenerate rather than manually
reordering). Run the code-generator to produce a correct manifest: re-run
controller-gen (or run the project's make manifests target) to regenerate
config/crd/bases/self-node-remediation.medik8s.io_selfnoderemediationconfigs.yaml
so that enableIncludeInRemediationLabel is placed in proper alphabetical order
alongside isSoftwareRebootEnabled and endpointHealthCheckUrl, then commit the
regenerated file.
| {{ if .EnableIncludeInRemediationLabel }} | ||
| - key: remediation.medik8s.io/include-in-remediation | ||
| operator: In | ||
| values: | ||
| - "true" | ||
| {{ end }} |
There was a problem hiding this comment.
YAML lint error is a false positive; minor indentation inconsistency.
False positive (line 39): The yamlint error is caused by {{ if ... }} / {{ end }} being invalid raw YAML. Since this is a Go template file, this error is expected and can be suppressed/ignored for this file.
Indentation inconsistency: The values: list item in the new expression uses 2-space indentation relative to values: (line 42), while the existing expression on line 37 places - "true" at the same column as values:. Both are valid YAML, but keeping them consistent improves readability.
✏️ Proposed fix for consistent indentation
{{ if .EnableIncludeInRemediationLabel }}
- key: remediation.medik8s.io/include-in-remediation
operator: In
values:
- - "true"
+ - "true"
{{ end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ if .EnableIncludeInRemediationLabel }} | |
| - key: remediation.medik8s.io/include-in-remediation | |
| operator: In | |
| values: | |
| - "true" | |
| {{ end }} | |
| {{ if .EnableIncludeInRemediationLabel }} | |
| - key: remediation.medik8s.io/include-in-remediation | |
| operator: In | |
| values: | |
| - "true" | |
| {{ end }} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 39-39: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/self-node-remediation-deamonset.yaml` around lines 38 - 43, This
block is a Go template so the yaml linter false-positive should be suppressed
and the indentation normalized: ensure the conditional block starting with "{{
if .EnableIncludeInRemediationLabel }}" and ending with "{{ end }}" remains
as-is (it's valid in templates), but align the list item under "values:" so "-
\"true\"" uses the same column alignment as the existing similar expression
(make the dash start directly under the first character of "values:"'s line for
consistency); additionally add a file-level linter exception or suppress rule
for Go template files so the yamlint warning is ignored for this template.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing tests covering syncConfigDaemonSet or daemonset affinity rendering
rg -n --type=go -C3 "EnableIncludeInRemediationLabel\|include-in-remediation\|syncConfigDaemonSet" \
--glob '*_test.go'Repository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Get an overview of the repository structure
find . -type f -name "*_test.go" | head -20Repository: medik8s/self-node-remediation
Length of output: 868
🏁 Script executed:
#!/bin/bash
# Check if there are any test files related to daemonset configuration
fd -e _test.go | grep -i daemonRepository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Look for test plan or PR description that might mention testing requirements
find . -type f \( -name "*.md" -o -name "*.txt" \) | head -20Repository: medik8s/self-node-remediation
Length of output: 1008
🏁 Script executed:
#!/bin/bash
# Search for all Go files to understand the project structure better
fd -e .go | grep -E "(config|daemon|remediation)" | head -20Repository: medik8s/self-node-remediation
Length of output: 1433
🏁 Script executed:
#!/bin/bash
# Search for daemonset rendering or template logic in the codebase
rg -n "daemonset|daemonSet|DaemonSet" --type=go | grep -v vendor | head -30Repository: medik8s/self-node-remediation
Length of output: 3563
🏁 Script executed:
#!/bin/bash
# Look for EnableIncludeInRemediationLabel in all files (not just tests)
rg -n "EnableIncludeInRemediationLabel" --type=go | grep -v vendorRepository: medik8s/self-node-remediation
Length of output: 759
🏁 Script executed:
#!/bin/bash
# Check the config controller test file to see what it covers
wc -l ./controllers/tests/config/selfnoderemediationconfig_controller_test.goRepository: medik8s/self-node-remediation
Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for render or template related files
fd -e .go | xargs rg -l "render\|template" | grep -v vendor | head -20Repository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Look at the syncConfigDaemonSet function to understand the implementation
sed -n '136,250p' ./controllers/selfnoderemediationconfig_controller.goRepository: medik8s/self-node-remediation
Length of output: 4448
🏁 Script executed:
#!/bin/bash
# Look at what happens with the data passed to template (around line 161)
sed -n '150,180p' ./controllers/selfnoderemediationconfig_controller.goRepository: medik8s/self-node-remediation
Length of output: 1601
🏁 Script executed:
#!/bin/bash
# Find where the template rendering happens
rg -n "template\|render" controllers/selfnoderemediationconfig_controller.go -B2 -A2Repository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check the test file to see what scenarios are tested
head -100 ./controllers/tests/config/selfnoderemediationconfig_controller_test.goRepository: medik8s/self-node-remediation
Length of output: 3595
🏁 Script executed:
#!/bin/bash
# Look for any render-related tests
find . -path ./vendor -prune -o -type f -name "*render*test*" -printRepository: medik8s/self-node-remediation
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Check what tests exist in the config controller test file more comprehensively
grep -n "It\|Context\|Describe" ./controllers/tests/config/selfnoderemediationconfig_controller_test.goRepository: medik8s/self-node-remediation
Length of output: 1007
🏁 Script executed:
#!/bin/bash
# Search for any test that might exercise EnableIncludeInRemediationLabel
rg -n "EnableIncludeInRemediationLabel\|include-in-remediation" . --type=go | grep -v vendorRepository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check the render test file to see what it covers
head -100 ./pkg/render/render_test.goRepository: medik8s/self-node-remediation
Length of output: 2503
🏁 Script executed:
#!/bin/bash
# Search in render test for any affinity or label-related tests
rg -n "affinity\|label\|enableinclude" ./pkg/render/render_test.go -iRepository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Get the full size and structure of render_test.go
wc -l ./pkg/render/render_test.go
grep -n "func Test\|It\|Context" ./pkg/render/render_test.goRepository: medik8s/self-node-remediation
Length of output: 256
🏁 Script executed:
#!/bin/bash
# Check if there are any tests that set EnableIncludeInRemediationLabel to true
grep -n "EnableIncludeInRemediationLabel\|include-in-remediation" ./controllers/tests/config/selfnoderemediationconfig_controller_test.goRepository: medik8s/self-node-remediation
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Look for the complete test cases that exercise daemonset creation
sed -n '101,167p' ./controllers/tests/config/selfnoderemediationconfig_controller_test.goRepository: medik8s/self-node-remediation
Length of output: 3259
🏁 Script executed:
#!/bin/bash
# Check the shared test utilities to see what the default config looks like
fd -type f -name "shared.go" | xargs grep -l "GenerateTestConfig\|SelfNodeRemediationConfig"Repository: medik8s/self-node-remediation
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Find and examine the shared test utilities
find . -path ./vendor -prune -o -type f -name "shared.go" -print | grep -v vendorRepository: medik8s/self-node-remediation
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Examine the shared test utilities
cat ./controllers/tests/shared/shared.goRepository: medik8s/self-node-remediation
Length of output: 2911
🏁 Script executed:
#!/bin/bash
# Search for what default value EnableIncludeInRemediationLabel uses in the shared test config
grep -A30 "GenerateTestConfig" ./controllers/tests/shared/shared.goRepository: medik8s/self-node-remediation
Length of output: 1436
Add tests for EnableIncludeInRemediationLabel daemonset affinity rendering.
The existing test suite exercises the default EnableIncludeInRemediationLabel = false path only. Tests are needed to verify that when EnableIncludeInRemediationLabel = true, the daemonset is created with the proper affinity/label selector for remediation.medik8s.io/include-in-remediation and that when false, the affinity/label selector is absent. Specifically, add test cases to selfnoderemediationconfig_controller_test.go covering the syncConfigDaemonSet rendering path with this flag enabled.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 39-39: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install/self-node-remediation-deamonset.yaml` around lines 38 - 43, Add unit
tests in selfnoderemediationconfig_controller_test.go that exercise
syncConfigDaemonSet for both values of EnableIncludeInRemediationLabel: create
one test case with EnableIncludeInRemediationLabel = true and assert the
rendered DaemonSet contains the nodeAffinity/nodeSelectorTerm for
remediation.medik8s.io/include-in-remediation (operator In, value "true"), and
another with EnableIncludeInRemediationLabel = false and assert that
affinity/label selector is absent; instantiate the config used by
syncConfigDaemonSet, call the function to render the DaemonSet, and use
assertions on the returned podSpec/affinity and label selector fields to
validate presence/absence.
1 participant
…ion" label in DaemonSet nodeAffinity configuration.
Why we need this PR
This will give ability to conditionally include labels in nodeAffinity configuration for DaemonSet.
Changes made
Include enableIncludeInRemediationLabel configuration option, with default value
falseto be compatible with current default configuration.Which issue(s) this PR fixes
Fixes #285
Test plan
Summary by CodeRabbit