(WIP) MCO-2279#6050
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen 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 |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
WalkthroughAdded clarifying SNO compatibility comments to five ImageModeStatusReporting test cases, changed the JSONPath used by IsSingleNodeTopology to read ChangesImageModeStatusReporting & test utilities
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/image_mode_status_reporting.go (1)
42-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit SNO protection to match coding guidelines.
The comment documents that this test won't run on SNO, and the test does skip when there are no worker nodes (Line 50-51). However, the coding guidelines require explicit SNO protection using one of these mechanisms:
[Skipped:SingleReplicaTopology]label in the test nameexutil.IsSingleNode()check withg.Skip()skipOnSingleNodeTopology()callThe current implicit protection (checking for worker nodes) works but isn't the standard pattern and could be fragile if refactored.
🔧 Suggested explicit SNO protection
Option 1: Add label to test name:
- // Will not run on SNO - g.It("MachineConfigNode properties should match the associated node properties when OCB is enabled in a custom MCP [apigroup:machineconfiguration.openshift.io]", func() { + g.It("MachineConfigNode properties should match the associated node properties when OCB is enabled in a custom MCP [apigroup:machineconfiguration.openshift.io][Skipped:SingleReplicaTopology]", func() {Option 2: Add explicit check at test start:
g.It("MachineConfigNode properties should match the associated node properties when OCB is enabled in a custom MCP [apigroup:machineconfiguration.openshift.io]", func() { + if exutil.IsSingleNode() { + g.Skip("Test requires multiple worker nodes") + } + // Create machine config client for testAs per coding guidelines: "Flag tests that expect multiple control-plane/master nodes, multiple worker nodes... Do NOT flag Single Node OpenShift (SNO) test compatibility if test is already protected by: test name includes
[Skipped:SingleReplicaTopology]label, test body containsexutil.IsSingleNode()check withg.Skip()..."🤖 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/image_mode_status_reporting.go` around lines 42 - 56, The test lacks explicit Single Node OpenShift (SNO) protection; keep the existing worker-MCP check (DoesMachineConfigPoolHaveMachines) but add one of the approved SNO guards before running runImageModeMCNTest: either add the `[Skipped:SingleReplicaTopology]` label to the g.It test name, or call exutil.IsSingleNode() with g.Skip(), or call skipOnSingleNodeTopology() at the start of the test body; place the guard immediately after creating machineConfigClient and before the worker-MCP check so the test follows the coding guideline and still calls runImageModeMCNTest(oc, machineConfigClient, "infra", "", false) only when appropriate.
♻️ Duplicate comments (2)
test/extended/image_mode_status_reporting.go (2)
74-88: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit SNO protection.
Same issue as the tests at lines 42-56 and 58-72: this test requires worker nodes and will skip on SNO, but lacks explicit SNO protection per coding guidelines. Consider adding
[Skipped:SingleReplicaTopology]label or anexutil.IsSingleNode()check.🤖 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/image_mode_status_reporting.go` around lines 74 - 88, This test lacks explicit Single Node (SNO) protection: add an SNO guard similar to the other tests by either tagging the test with `[Skipped:SingleReplicaTopology]` in the g.It description or programmatically skipping when single-node, e.g. call exutil.IsSingleNode() before the worker-MCP check; update the test case that calls runImageModeMCNTest(oc, machineConfigClient, "infra", "90-infra-testfile", false) so it returns/skips on single-replica topology and keep the existing DoesMachineConfigPoolHaveMachines(machineConfigClient, "worker") check.
58-72: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit SNO protection.
Same issue as the test at lines 42-56: this test requires worker nodes and will skip on SNO, but lacks explicit SNO protection per coding guidelines. Consider adding
[Skipped:SingleReplicaTopology]label or anexutil.IsSingleNode()check.🤖 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/image_mode_status_reporting.go` around lines 58 - 72, The test "MachineConfigNode conditions should properly transition..." lacks explicit Single Node OpenShift (SNO) protection; add SNO gating before it runs by either adding the [Skipped:SingleReplicaTopology] label to the g.It declaration or inserting an explicit exutil.IsSingleNode() check at the top of the test body (before calling DoesMachineConfigPoolHaveMachines and runImageModeMCNTest) to skip the test on SNO clusters; reference the test by its g.It description and the functions DoesMachineConfigPoolHaveMachines and runImageModeMCNTest when making the change.
🤖 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.
Outside diff comments:
In `@test/extended/image_mode_status_reporting.go`:
- Around line 42-56: The test lacks explicit Single Node OpenShift (SNO)
protection; keep the existing worker-MCP check
(DoesMachineConfigPoolHaveMachines) but add one of the approved SNO guards
before running runImageModeMCNTest: either add the
`[Skipped:SingleReplicaTopology]` label to the g.It test name, or call
exutil.IsSingleNode() with g.Skip(), or call skipOnSingleNodeTopology() at the
start of the test body; place the guard immediately after creating
machineConfigClient and before the worker-MCP check so the test follows the
coding guideline and still calls runImageModeMCNTest(oc, machineConfigClient,
"infra", "", false) only when appropriate.
---
Duplicate comments:
In `@test/extended/image_mode_status_reporting.go`:
- Around line 74-88: This test lacks explicit Single Node (SNO) protection: add
an SNO guard similar to the other tests by either tagging the test with
`[Skipped:SingleReplicaTopology]` in the g.It description or programmatically
skipping when single-node, e.g. call exutil.IsSingleNode() before the worker-MCP
check; update the test case that calls runImageModeMCNTest(oc,
machineConfigClient, "infra", "90-infra-testfile", false) so it returns/skips on
single-replica topology and keep the existing
DoesMachineConfigPoolHaveMachines(machineConfigClient, "worker") check.
- Around line 58-72: The test "MachineConfigNode conditions should properly
transition..." lacks explicit Single Node OpenShift (SNO) protection; add SNO
gating before it runs by either adding the [Skipped:SingleReplicaTopology] label
to the g.It declaration or inserting an explicit exutil.IsSingleNode() check at
the top of the test body (before calling DoesMachineConfigPoolHaveMachines and
runImageModeMCNTest) to skip the test on SNO clusters; reference the test by its
g.It description and the functions DoesMachineConfigPoolHaveMachines and
runImageModeMCNTest when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a834512b-7047-44a2-bb0b-884f9ee71cef
📒 Files selected for processing (1)
test/extended/image_mode_status_reporting.go
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-single-node-disruptive |
|
@isabella-janssen: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/23a99e80-551c-11f1-91c5-e3cba9646b62-0 |
1 participant
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit