NO-ISSUE: Fix setArchitectureAndCheckStatus corrupting multi-label annotations - #6347
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@ptalgulk01: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughThe change adds JSON-safe autoscaler label patching to ChangesAutoscaler architecture label handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/machineset.go`:
- Around line 714-717: Update SetArchAnnotation to JSON-encode labels with
encoding/json.Marshal before constructing the patch payload, and insert the
marshaled value rather than the raw string. Preserve the existing annotation
path and Patch call, and return any marshaling error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a41dea45-0c0c-419d-a86d-3bdb1c7c652d
📒 Files selected for processing (2)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.go
| // SetArchAnnotation sets the raw capacity labels annotation for this MachineSet. | ||
| // Use this instead of SetArchitecture when the value contains multiple comma-separated | ||
| // labels (e.g. "kubernetes.io/arch=amd64,topology.ebs.csi.aws.com/zone=eu-central-1a"). | ||
| func (ms MachineSet) SetArchAnnotation(labels string) error { |
There was a problem hiding this comment.
We are not actually setting the architecture, there are more things that we can set in this label.
Maybe SetAutoscalerLabels would be a better name?
Having SetArchAnnotation and SetArchitecture can be confusing.
SetArchitecture now should call SetArchAnnotation instead of duplicating the annotation name.
|
/test tls-pqc-readiness Ignore this test, I'm just trying to show it's stable across PRs before making it required. |
9a7fc21 to
f14e1e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/machineset.go`:
- Around line 709-721: Update GetArchitecture to parse comma-separated
autoscaler label entries and pass only the value of the exact kubernetes.io/arch
label to architecture.FromString. Preserve existing behavior for single-label
annotations and ensure unrelated labels are not included in the architecture
value; alternatively, restrict SetAutoscalerLabels to single-label input if that
is the intended contract.
- Line 331: Update SetArchitecture to normalize its arch input before calling
SetAutoscalerLabels, avoiding unconditional addition of the kubernetes.io/arch=
prefix when the value already contains it. Preserve correctly formatted inputs,
including values with additional comma-separated labels, without producing a
duplicate prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b49cffed-8d43-425b-b271-053114a4236f
📒 Files selected for processing (2)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.go
f14e1e4 to
92a24ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/extended-priv/mco_bootimages.go`:
- Around line 1055-1057: The autoscaler-label payload is exposed by the
description and logger calls around clonedMS.SetAutoscalerLabels(labels). Stop
interpolating labels into test output, and instead log only a redacted
confirmation or the specific architecture value while preserving the
label-setting operation and success assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b07d3809-6107-40ea-9f5e-4596f8c7c0ea
📒 Files selected for processing (2)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.go
| exutil.By(fmt.Sprintf("Set a %s architecture in the cloned machineset", labels)) | ||
| o.Expect(clonedMS.SetAutoscalerLabels(labels)).To(o.Succeed(), "Error setting architecture %s in %s", labels, clonedMS) | ||
| logger.Infof("Architecture %s set in %s\n", labels, clonedMS) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Avoid emitting the raw autoscaler-label payload.
labels may contain arbitrary comma-separated labels, including values such as kubernetes.io/hostname=...; both calls currently write the full payload to test output. Log only a redacted confirmation or the architecture value.
Suggested change
- exutil.By(fmt.Sprintf("Set a %s architecture in the cloned machineset", labels))
+ exutil.By("Set architecture labels in the cloned machineset")
o.Expect(clonedMS.SetAutoscalerLabels(labels)).To(o.Succeed(), "Error setting architecture %s in %s", labels, clonedMS)
- logger.Infof("Architecture %s set in %s\n", labels, clonedMS)
+ logger.Infof("Architecture labels set in the cloned machineset")As per coding guidelines, “Flag logging that may expose ... internal hostnames ... or customer data.”
📝 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.
| exutil.By(fmt.Sprintf("Set a %s architecture in the cloned machineset", labels)) | |
| o.Expect(clonedMS.SetAutoscalerLabels(labels)).To(o.Succeed(), "Error setting architecture %s in %s", labels, clonedMS) | |
| logger.Infof("Architecture %s set in %s\n", labels, clonedMS) | |
| exutil.By("Set architecture labels in the cloned machineset") | |
| o.Expect(clonedMS.SetAutoscalerLabels(labels)).To(o.Succeed(), "Error setting architecture %s in %s", labels, clonedMS) | |
| logger.Infof("Architecture labels set in the cloned machineset") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/extended-priv/mco_bootimages.go` around lines 1055 - 1057, The
autoscaler-label payload is exposed by the description and logger calls around
clonedMS.SetAutoscalerLabels(labels). Stop interpolating labels into test
output, and instead log only a redacted confirmation or the specific
architecture value while preserving the label-setting operation and success
assertion.
Source: Coding guidelines
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01, sergiordlr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified later |
|
@ptalgulk01: DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified later @ptalgulk01 |
|
@ptalgulk01: This PR has been marked to be verified later by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
verified
|
@ptalgulk01: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-machine-config-operator-release-4.23-periodics-e2e-vsphere-mco-tp-longduration-2of2/2082047377660710912
Summary by CodeRabbit
kubernetes.io/arch=prefix.