Migrate remaining TCs from mco.go to mco.go, mco_units.go, mco_kubele…#6024
Migrate remaining TCs from mco.go to mco.go, mco_units.go, mco_kubele…#6024ptalgulk01 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
WalkthroughAdds multiple disruptive long-duration Ginkgo tests to the extended MCO suite plus a MachineConfig template and helper: tests cover Ignition-version error handling, operator/MCP health checks, Azure platform status consistency, controllerconfig sync after Infrastructure labeling, ImageRegistry-capability behavior, node log scanning, nmstate config persistence, and systemd unit lifecycle across MachineConfig updates. ChangesMCO Extended Test Suite Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 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 |
![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
🧹 Nitpick comments (2)
test/extended-priv/mco_units.go (1)
385-402: 💤 Low valueMinor: typo
nsmtatein the error messages and duplicated restart logic.The same
systemctl restart nmstateplus error-string assertion is repeated in the deferred cleanup (Lines 388-390) and the test body (Lines 400-401), and both error strings misspellnmstateasnsmtate. Consider extracting a tiny local helper and fixing the typo while you're touching it.Proposed tweak
+ restartNMState := func() { + logger.Infof("Restarting nmstate service") + _, err := node.DebugNodeWithChroot("systemctl", "restart", "nmstate") + o.Expect(err).NotTo(o.HaveOccurred(), "Error restarting the nmstate service in node %s", node.GetName()) + } defer func() { nmstateConfigRemote.Rm("-f") nmstateConfigAppliedRemote.Rm("-f") - logger.Infof("Restarting nmstate service") - _, err := node.DebugNodeWithChroot("systemctl", "restart", "nmstate") - o.Expect(err).NotTo(o.HaveOccurred(), "Error restarting the nsmtate service in node %s", node.GetName()) + restartNMState() }() ... exutil.By(fmt.Sprintf("Restart nmstate service in node %s", node.GetName())) - _, err := node.DebugNodeWithChroot("systemctl", "restart", "nmstate") - o.Expect(err).NotTo(o.HaveOccurred(), "Error restarting the nsmtate service in node %s", node.GetName()) + restartNMState()🤖 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_units.go` around lines 385 - 402, The deferred cleanup and the test body both call systemctl restart nmstate with duplicated logic and misspell "nmstate" as "nsmtate"; extract a small helper (e.g., restartNmstate or restartNmstateWithAssert) that calls node.DebugNodeWithChroot("systemctl","restart","nmstate") and performs the o.Expect(err).NotTo(o.HaveOccurred(), ...) assertion using the correct service name, then replace the inline restart calls in the defer func and the main test flow to call that helper; update any related log messages (logger.Infof / error strings) to use "nmstate" consistently and remove the duplicate restart logic.test/extended-priv/mco.go (1)
178-201: 💤 Low valueOptional: structural compare instead of string compare for platformStatus.
mccPlatformStatusandinfraPlatformStatusare both produced byjsonpathextraction, so ordering should be stable in practice, but a single field-order or whitespace difference between the two paths would make this assertion fail in a way that is hard to debug. Decoding both intomap[string]interface{}(or unmarshaling into the same typed struct) and comparing withreflect.DeepEqual/ Gomega'so.Equalon the maps would be more robust.🤖 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.go` around lines 178 - 201, The test currently compares mccPlatformStatus and infraPlatformStatus as raw strings which is brittle; instead decode both JSONPath results into the same Go structure (e.g., map[string]interface{} via json.Unmarshal) — parse mccPlatformStatus into a variable like jsonMccPlatformStatus (already present) and likewise unmarshal infraPlatformStatus into jsonInfraPlatformStatus, assert both unmarshals succeed, then compare the parsed maps using o.Expect(jsonMccPlatformStatus).To(o.Equal(jsonInfraPlatformStatus)) (or reflect.DeepEqual) so ordering/whitespace differences won’t cause false failures; update the check in the test function that currently does o.Expect(mccPlatformStatus).To(o.Equal(infraPlatformStatus)).
🤖 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/testdata/files/change-worker-ign-version.yaml`:
- Around line 12-17: The YAML has kernelArguments at the top-level instead of
under the MachineConfig spec; move the kernelArguments sequence into the spec
block (so it is a child of spec alongside config), or remove the kernelArguments
entry altogether if it's not needed for this invalid-ignition-version test;
target the kernelArguments key and the spec mapping in the YAML so the
MachineConfig schema is valid.
---
Nitpick comments:
In `@test/extended-priv/mco_units.go`:
- Around line 385-402: The deferred cleanup and the test body both call
systemctl restart nmstate with duplicated logic and misspell "nmstate" as
"nsmtate"; extract a small helper (e.g., restartNmstate or
restartNmstateWithAssert) that calls
node.DebugNodeWithChroot("systemctl","restart","nmstate") and performs the
o.Expect(err).NotTo(o.HaveOccurred(), ...) assertion using the correct service
name, then replace the inline restart calls in the defer func and the main test
flow to call that helper; update any related log messages (logger.Infof / error
strings) to use "nmstate" consistently and remove the duplicate restart logic.
In `@test/extended-priv/mco.go`:
- Around line 178-201: The test currently compares mccPlatformStatus and
infraPlatformStatus as raw strings which is brittle; instead decode both
JSONPath results into the same Go structure (e.g., map[string]interface{} via
json.Unmarshal) — parse mccPlatformStatus into a variable like
jsonMccPlatformStatus (already present) and likewise unmarshal
infraPlatformStatus into jsonInfraPlatformStatus, assert both unmarshals
succeed, then compare the parsed maps using
o.Expect(jsonMccPlatformStatus).To(o.Equal(jsonInfraPlatformStatus)) (or
reflect.DeepEqual) so ordering/whitespace differences won’t cause false
failures; update the check in the test function that currently does
o.Expect(mccPlatformStatus).To(o.Equal(infraPlatformStatus)).
🪄 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: Enterprise
Run ID: cf56ce46-bf1e-4f69-95b4-ac1b44b9d849
📒 Files selected for processing (4)
test/extended-priv/mco.gotest/extended-priv/mco_kubelet_and_containerruntime.gotest/extended-priv/mco_units.gotest/extended-priv/testdata/files/change-worker-ign-version.yaml
ptalgulk01
force-pushed
the
migrate-mco-remaining-tcs
branch
from
d0e6c2d to
64c63d2
Compare
…t_and_containerruntime.go - Add 6 general MCO TCs (42347, 43124, 43726, 63868, 68695, 75258) to mco.go - Add TC 72025 (nmstate keeps service yamls) to mco_units.go - Add TC 74540 (kubelet dependency issue after reboot) to mco_kubelet_and_containerruntime.go - Add change-worker-ign-version.yaml template for TC 43124 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ptalgulk01
force-pushed
the
migrate-mco-remaining-tcs
branch
from
64c63d2 to
df0c8fa
Compare
|
@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. |
1 participant
Summary by CodeRabbit