docs: add system requirements section to README#3350
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe README gains a new "Development Requirements" section listing prerequisite tools (Go, Make, Podman/Docker, Node.js) with a reference to Troubleshooting. The Troubleshooting section receives clarified ChangesREADME documentation updates
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
🤖 Finished Review · ✅ Success · Started 10:29 AM UTC · Completed 10:36 AM UTC |
|
Looks good to me Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsHigh
Medium
Low
Info
Previous run (4)ReviewFindingsHigh
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| Run: | ||
|
|
||
| ```bash | ||
| systemctl disable --now --user podman.service |
There was a problem hiding this comment.
[high] technical accuracy
The systemctl disable --now --user podman.service command disables podman.service in user scope, but the existing Troubleshooting section (lines 66-73) explains that problems arise from Podman running as a system service (rootful). The disable step must target system scope. The command would only disable a user-level service unit, leaving the problematic system-level services running.
Suggested fix: Remove the --user flag from the disable command and include both socket and service: systemctl disable --now podman.socket podman.service. Or reference the existing Troubleshooting section 2.1 rather than duplicating commands with different semantics.
|
|
||
| ```bash | ||
| systemctl disable --now --user podman.service | ||
| systemctl enable --user --now podman.socket |
There was a problem hiding this comment.
[low] internal consistency
The enable command only enables podman.socket, while the existing Troubleshooting section enables both podman.socket and podman.service. While socket-only activation is valid (and matches AGENTS.md), the contradictory instructions within README.md may confuse contributors.
Suggested fix: Align commands with the existing Troubleshooting section, or consolidate by referencing section 2.1 instead of duplicating divergent instructions.
|
I implemented the podman command follow-up from the review comments in commit 4660705: aligned the top README Podman startup/stop instructions with the existing troubleshooting section by removing where needed and using socket+service for both disable/enable commands. Let me know if anything else is still inconsistent. |
|
🤖 Finished Review · ✅ Success · Started 11:29 AM UTC · Completed 11:36 AM UTC |
| Run: | ||
|
|
||
| ```bash | ||
| systemctl disable --now podman.socket podman.service |
There was a problem hiding this comment.
[high] technical accuracy
The new Development Requirements section instructs users to run Podman as a system service (systemctl enable --now, without --user), and removes --user from the existing troubleshooting section. This directly contradicts the existing README troubleshooting text which warns that system services cause issues, and contradicts AGENTS.md line 73 which says to use user services.
Suggested fix: Keep --user in the systemctl commands in both the new Development Requirements section and the existing troubleshooting section to match the rootless/user-service approach documented elsewhere.
| - Make | ||
| - Podman or Docker (for acceptance tests and container-based workflows) | ||
| - Node.js 18+ (for `tekton-lint`) | ||
|
|
There was a problem hiding this comment.
[medium] internal consistency
The disable-then-enable sequence for Podman system services is odd (systemctl restart would be more appropriate) and contradicts the troubleshooting section instruction to disable system services and use user services instead.
Suggested fix: Clarify the intent: if restarting, use systemctl restart. If the goal is user services, use the --user flag.
|
|
||
| Before building or testing `ec`, install the following tools: | ||
|
|
||
| - Go 1.25.8+ (matching `go.mod`) |
There was a problem hiding this comment.
[low] technical accuracy
The stated Go version Go 1.25.8+ matches go.mod and is internally consistent.
|
Implemented a follow-up follow-up on the Podman command consistency from review: restored user-scope Podman startup steps in README and aligned the development requirements section to |
|
🤖 Finished Review · ✅ Success · Started 1:27 PM UTC · Completed 1:33 PM UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
14-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the Podman setup aligned with the rootless flow.
The
--userdisable/enable sequence here does not address the rootful Podman service that the troubleshooting section says causes acceptance-test failures. Please reference that existing recipe instead of documenting a second, inconsistent variant.🤖 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 `@README.md` around lines 14 - 31, The Podman setup instructions in the Development Requirements section are inconsistent with the existing troubleshooting documentation elsewhere in the README. Remove the current systemctl disable/enable command sequence (systemctl disable --now --user podman.socket podman.service followed by systemctl enable --user --now podman.socket podman.service) and replace it with a reference to the existing rootless Podman setup recipe or troubleshooting section that already addresses the rootful vs rootless configuration issues properly. This ensures users are directed to a single, consistent source of truth for Podman configuration rather than having duplicate and potentially conflicting instructions.
🤖 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.
Duplicate comments:
In `@README.md`:
- Around line 14-31: The Podman setup instructions in the Development
Requirements section are inconsistent with the existing troubleshooting
documentation elsewhere in the README. Remove the current systemctl
disable/enable command sequence (systemctl disable --now --user podman.socket
podman.service followed by systemctl enable --user --now podman.socket
podman.service) and replace it with a reference to the existing rootless Podman
setup recipe or troubleshooting section that already addresses the rootful vs
rootless configuration issues properly. This ensures users are directed to a
single, consistent source of truth for Podman configuration rather than having
duplicate and potentially conflicting instructions.
| - Node.js 18+ (for `tekton-lint`) | ||
|
|
||
| On Fedora/RHEL systems, acceptance tests often require Podman to run as a user service. | ||
| Run: |
There was a problem hiding this comment.
[medium] logic-error
The new Development Requirements section instructs users to run systemctl disable --now --user podman.socket podman.service followed by systemctl enable --user --now podman.socket podman.service. The disable command targets the user service (--user flag), which stops/disables the rootless podman service before immediately re-enabling it — effectively a restart/bounce. This is semantically different from the existing Troubleshooting section (line 73), which runs systemctl disable --now podman.socket podman.service (without --user) to disable the system (rootful) service, then enables the user service. If the intent is to migrate from rootful to rootless (matching the troubleshooting pattern), the --user flag on the disable command is incorrect.
Suggested fix: Either (a) remove the --user flag from the disable command so it disables the system service (matching the troubleshooting section pattern), or (b) if the intent is to restart the user service, use systemctl restart --user podman.socket podman.service and clarify the prose.
| It is advised to do a reboot afterwards. | ||
|
|
||
| To start Podman for the user: | ||
| To start Podman: |
There was a problem hiding this comment.
[low] internal-consistency
The diff changes 'To start Podman for the user:' to 'To start Podman:' in the Troubleshooting section, dropping the 'for the user' qualifier. However, the command that follows uses the --user flag. Removing 'for the user' makes the heading less precise.
fullsend-ai-review
Bot
added
the
requires-manual-review
|
Implemented the remaining inline follow-up from fullsend on in commit . Changes: (1) made the Development Requirements Podman bootstrap disable command target the system service () to match the existing troubleshooting flow, and (2) restored wording to . |
|
Follow-up update re-commented on pushed as commit 34c03f2: Development Requirements now disables the system Podman service () before enabling user-scope Podman, and the troubleshooting heading now says . |
|
Follow-up update for #3350: commit 34c03f2 restores the existing Podman flow in README. It now disables the rootful Podman system service ( |
|
🤖 Finished Review · ✅ Success · Started 2:26 PM UTC · Completed 2:35 PM UTC |
| Run: | ||
|
|
||
| ```bash | ||
| systemctl disable --now podman.socket podman.service |
There was a problem hiding this comment.
[medium] technical-accuracy
The instructions tell users to run systemctl disable --now podman.socket podman.service (system-level) before enabling the user service. This is potentially destructive on shared or multi-user systems and inconsistent with AGENTS.md guidance which omits the disable step.
Suggested fix: Add a warning that this disables the system-wide Podman service and should only be done on single-user development machines, or align with AGENTS.md by omitting the disable step.
| Consult the [documentation][docs] for available sub-commands, descriptions and | ||
| examples of use. | ||
|
|
||
| ## Development Requirements |
There was a problem hiding this comment.
[medium] documentation-organization
The new Development Requirements section duplicates Podman setup instructions that already exist in the Troubleshooting section (lines 65-82), creating maintenance burden.
Suggested fix: Consolidate: either move Podman setup into this section and cross-reference from Troubleshooting, or remove from this section and cross-reference Troubleshooting.
| systemctl enable --user --now podman.socket podman.service | ||
| ``` | ||
|
|
||
| then run the test/build commands below. |
There was a problem hiding this comment.
[low] clarity
The phrase then run the test/build commands below is vague and does not specify which section it refers to.
Suggested fix: Replace with: then proceed to the Building or Testing sections below.
fullsend-ai-review
Bot
removed
the
requires-manual-review
|
Implemented the follow-up in (commit ) to resolve the Podman command feedback: removed duplicate system requirements Podman command snippet and referenced the existing Troubleshooting section instead, clarifying wording/flow. ❱ �[1mtekton-lint�[0m ❱ �[1mgo-mod-lint�[0m |
|
Implemented the follow-up in commit
Validation run on this branch: |
|
🤖 Finished Review · ✅ Success · Started 12:19 AM UTC · Completed 12:29 AM UTC |
fullsend-ai-review
Bot
added
the
ready-for-merge
1 participant
Closes #3343
Summary
Validation