Skip to content

cluster: reliably execute commands#1176

Open
achuzhoy wants to merge 1 commit intorh-ecosystem-edge:mainfrom
achuzhoy:execcmd
Open

cluster: reliably execute commands#1176
achuzhoy wants to merge 1 commit intorh-ecosystem-edge:mainfrom
achuzhoy:execcmd

Conversation

@achuzhoy
Copy link
Copy Markdown
Collaborator

@achuzhoy achuzhoy commented Feb 12, 2026

Previous way of executing commands against nodes
doesn't seem to work properly anymore

Summary by CodeRabbit

  • Tests
    • Improved image-based upgrade tests by simplifying output validation to a single-result check and updating the command execution path, reducing flakiness and making NTP source verification more reliable.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 12, 2026

📝 Walkthrough

Walkthrough

Replaces line-by-line stdout processing with a single concatenated output string for NTP source verification and swaps the command helper from ExecCmdWithStdoutWithRetries to ExecCommandOnSNOWithRetries.

Changes

Cohort / File(s) Summary
NTP Source Verification Refactor
tests/lca/imagebasedupgrade/mgmt/upgrade/tests/e2e-upgrade-test.go
Replaces per-line stdout iteration with a single-string cmdOutput containment check for each NTP source; changes command helper from ExecCmdWithStdoutWithRetries to ExecCommandOnSNOWithRetries.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • trewest
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'cluster: reliably execute commands' is clearly related to the main change—replacing command execution methods to use a more reliable helper function (ExecCommandOnSNOWithRetries instead of ExecCmdWithStdoutWithRetries).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

Command failed


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@tests/internal/cluster/cluster.go`:
- Line 156: The hostnameCmd slice is incorrectly a single-element slice, so
ExecCommand will search for an executable named "chroot /rootfs sh -c 'printf
$(hostname)'"; update hostnameCmd to be a proper argv slice (e.g.
[]string{"sh","-c","chroot /rootfs sh -c 'printf $(hostname)'"} or split into
[]string{"chroot","/rootfs","sh","-c","printf $(hostname)"}) so ExecCommand
receives separate args, and leave the variable name hostnameCmd and the call
site to ExecCommand unchanged.

Comment thread tests/internal/cluster/cluster.go Outdated
@achuzhoy achuzhoy force-pushed the execcmd branch 2 times, most recently from 8c7c77b to 6c28185 Compare February 12, 2026 05:23
Previous way of executing commands against nodes
doesn't seem to work properly anymore

Signed-off-by: Alexander Chuzhoy <achuzhoy@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant