chore: updating home dirs to not use root#185
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPlumbs customizable container home directories across evstack, dataavailability, and cosmos builders/nodes, adds DefaultHomeDir helpers, and updates container user handling and evmsingle default image UIDGID. ChangesHomeDir plumbing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/docker/evstack/evmsingle/config.go`:
- Around line 27-29: DefaultImage sets UIDGID to "10001:10001" but the container
runtime user isn't set, causing permission mismatches; update CreateContainer to
set container.Config.User = image.UIDGID (where image is the container.Image
passed into CreateContainer) so the runtime user matches the volume owner
configured by SetOwner(), ensuring DefaultImage(), UIDGID, CreateContainer,
container.Config and SetOwner are consistent; alternatively document in
DefaultImage that the upstream image already runs as UID 10001 if you choose not
to set User programmatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37c9b244-832b-4869-a619-7ec6eb759c2e
📒 Files selected for processing (2)
framework/docker/evstack/evmsingle/config.goframework/docker/evstack/evmsingle/node.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/docker/cosmos/chain_builder.go (1)
183-203: Consider copyinghomeDirinNewChainBuilderFromChain.When creating a builder from an existing chain, other configuration values are copied (e.g.,
Name,ChainID,Bin), buthomeDiris not included. If the original chain used a custom home directory, the new builder will use the default instead.This may be intentional, but worth verifying whether consistency is desired here.
💡 Suggested addition if homeDir should be preserved
You would need to store
homeDirinChainConfigand copy it:// After line 202, add: WithHomeDir(cfg.HomeDir)This requires adding a
HomeDirfield toChainConfigas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/docker/cosmos/chain_builder.go` around lines 183 - 203, NewChainBuilderFromChain currently copies many fields from Chain.Config but omits homeDir; add a HomeDir string field to the ChainConfig struct and update NewChainBuilderFromChain to call WithHomeDir(cfg.HomeDir) on the ChainBuilder so the builder preserves custom home directories; ensure the ChainBuilder has a WithHomeDir method (or add one) to accept and store the HomeDir value.framework/docker/evstack/spamoor/node.go (1)
49-52: Consider adding aDefaultHomeDir()helper for consistency.The hardcoded default
"/home/spamoor"works correctly, but other modules in this PR (reth, evmsingle, dataavailability, evstack) define aDefaultHomeDir()function. Adding one here would improve maintainability and consistency.♻️ Suggested refactor
Add to this file or a separate
config.go:// DefaultHomeDir returns the default home directory for spamoor containers. func DefaultHomeDir() string { return "/home/spamoor" }Then update the defaulting logic:
func newNode(ctx context.Context, cfg Config, testName string, index int, name string, homeDir string) (*Node, error) { if homeDir == "" { - homeDir = "/home/spamoor" + homeDir = DefaultHomeDir() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/docker/evstack/spamoor/node.go` around lines 49 - 52, Add a DefaultHomeDir() helper that returns "/home/spamoor" (e.g., in this file or a new config.go) and update newNode to call DefaultHomeDir() instead of hardcoding the string; specifically, add the function DefaultHomeDir() and change the defaulting branch in newNode (the homeDir == "" check) to set homeDir = DefaultHomeDir() so the module follows the same pattern as reth/evmsingle/dataavailability/evstack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/docker/cosmos/chain_builder.go`:
- Around line 183-203: NewChainBuilderFromChain currently copies many fields
from Chain.Config but omits homeDir; add a HomeDir string field to the
ChainConfig struct and update NewChainBuilderFromChain to call
WithHomeDir(cfg.HomeDir) on the ChainBuilder so the builder preserves custom
home directories; ensure the ChainBuilder has a WithHomeDir method (or add one)
to accept and store the HomeDir value.
In `@framework/docker/evstack/spamoor/node.go`:
- Around line 49-52: Add a DefaultHomeDir() helper that returns "/home/spamoor"
(e.g., in this file or a new config.go) and update newNode to call
DefaultHomeDir() instead of hardcoding the string; specifically, add the
function DefaultHomeDir() and change the defaulting branch in newNode (the
homeDir == "" check) to set homeDir = DefaultHomeDir() so the module follows the
same pattern as reth/evmsingle/dataavailability/evstack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e153db36-0a1e-43f5-bc5a-089a252392ca
📒 Files selected for processing (15)
framework/docker/cosmos/chain_builder.goframework/docker/dataavailability/config.goframework/docker/dataavailability/network_builder.goframework/docker/dataavailability/node.goframework/docker/evstack/chain_builder.goframework/docker/evstack/config.goframework/docker/evstack/evmsingle/builder.goframework/docker/evstack/evmsingle/config.goframework/docker/evstack/evmsingle/node.goframework/docker/evstack/node.goframework/docker/evstack/reth/builder.goframework/docker/evstack/reth/config.goframework/docker/evstack/reth/node.goframework/docker/evstack/spamoor/builder.goframework/docker/evstack/spamoor/node.go
tty47
approved these changes
Mar 19, 2026
Move homeDir from bare function parameter into Config structs for dataavailability and evstack packages, matching the pattern already used by evmsingle and reth. Add DefaultHomeDir() to spamoor for consistency. Fix spaces-to-tabs formatting across several files.
chatton
force-pushed
the
cian/update-root-user
branch
from
5531019 to
ad70f93
Compare
Contributor
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 (3)
framework/docker/evstack/chain_builder.go (1)
154-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
HomeDirinto chain-level config inBuild().
newNode()receivesb.homeDir, butBuild()drops it fromChain.cfg. That can silently lose the override for any path that readschain.cfg.HomeDir.Suggested fix
return &Chain{ cfg: Config{ Logger: b.logger, DockerClient: b.dockerClient, DockerNetworkID: b.dockerNetworkID, ChainID: b.chainID, Env: b.env, Bin: b.binaryName, AggregatorPassphrase: b.aggregatorPassphrase, Image: *b.dockerImage, + HomeDir: b.homeDir, }, log: b.logger, nodes: nodes, }, nil🤖 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 `@framework/docker/evstack/chain_builder.go` around lines 154 - 163, The Chain config built in Build() is missing the HomeDir field so any override passed via b.homeDir isn't propagated; update the Config literal assigned to cfg in Build() (the Config used to construct Chain.cfg) to include HomeDir: b.homeDir so that Chain.cfg.HomeDir is set, keeping newNode() unchanged (it already receives b.homeDir). Ensure you set the HomeDir field on the Config struct used when creating the Chain so downstream code reading Chain.cfg.HomeDir sees the override.framework/docker/dataavailability/network_builder.go (2)
160-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStore
HomeDirin the Network's Config.The
Buildmethod doesn't includeHomeDirin theNetwork.cfgthat it creates. This field is needed so thatNewNetworkBuilderFromNetworkcan later retrieve the home directory when adding nodes dynamically. Without this, the fix suggested in theNewNetworkBuilderFromNetworkcomment cannot work.🔧 Proposed fix
return &Network{ cfg: Config{ Logger: b.logger, DockerClient: b.dockerClient, DockerNetworkID: b.dockerNetworkID, ChainID: b.chainID, Env: b.env, Bin: b.binaryName, Image: *b.dockerImage, + HomeDir: b.homeDir, }, log: b.logger, nodeMap: nodeMap, nextNodeIdx: len(nodeMap), // start from the number of initial nodes testName: b.testName, // store original test name for dynamic node naming }, nil🤖 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 `@framework/docker/dataavailability/network_builder.go` around lines 160 - 169, The Network built in Build() omits the Config.HomeDir so NewNetworkBuilderFromNetwork can't retrieve the home directory later; update the Network construction in Build to set Config.HomeDir (e.g., add HomeDir: b.homeDir) alongside Logger, DockerClient, DockerNetworkID, ChainID, Env, Bin and Image so the Network.cfg contains the home directory for downstream use by NewNetworkBuilderFromNetwork.
63-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
homeDirwhen creating builder from existing network.
NewNetworkBuilderFromNetworkdoesn't copyhomeDirfrom the original network configuration. When this function is used to add nodes dynamically (viaNetwork.AddNodes), new nodes will useDefaultHomeDir()instead of inheriting the home directory from existing nodes, creating inconsistent state within a single network.For example:
- Create network with
WithHomeDir("/custom/path")- Initial nodes correctly use
/custom/path- Later call
AddNodesto add more nodes- New nodes use
/home/celestia(default) instead of/custom/path🔧 Proposed fix
func NewNetworkBuilderFromNetwork(network *Network) *NetworkBuilder { return &NetworkBuilder{ testName: network.testName, logger: network.log, dockerClient: network.cfg.DockerClient, dockerNetworkID: network.cfg.DockerNetworkID, dockerImage: &network.cfg.Image, additionalStartArgs: network.cfg.AdditionalStartArgs, env: network.cfg.Env, chainID: network.cfg.ChainID, binaryName: network.cfg.Bin, + homeDir: network.cfg.HomeDir, } }🤖 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 `@framework/docker/dataavailability/network_builder.go` around lines 63 - 74, NewNetworkBuilderFromNetwork currently omits copying the homeDir from the source Network, so builders created for AddNodes end up using DefaultHomeDir(); update NewNetworkBuilderFromNetwork to set the NetworkBuilder.homeDir from network.cfg.HomeDir (or the existing configured value) when constructing the builder so new nodes inherit the original home directory instead of falling back to DefaultHomeDir(); modify the constructor that returns &NetworkBuilder{...} to include homeDir: network.cfg.HomeDir (or equivalent field) while leaving other behavior unchanged.
🤖 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 `@framework/docker/dataavailability/network_builder.go`:
- Around line 160-169: The Network built in Build() omits the Config.HomeDir so
NewNetworkBuilderFromNetwork can't retrieve the home directory later; update the
Network construction in Build to set Config.HomeDir (e.g., add HomeDir:
b.homeDir) alongside Logger, DockerClient, DockerNetworkID, ChainID, Env, Bin
and Image so the Network.cfg contains the home directory for downstream use by
NewNetworkBuilderFromNetwork.
- Around line 63-74: NewNetworkBuilderFromNetwork currently omits copying the
homeDir from the source Network, so builders created for AddNodes end up using
DefaultHomeDir(); update NewNetworkBuilderFromNetwork to set the
NetworkBuilder.homeDir from network.cfg.HomeDir (or the existing configured
value) when constructing the builder so new nodes inherit the original home
directory instead of falling back to DefaultHomeDir(); modify the constructor
that returns &NetworkBuilder{...} to include homeDir: network.cfg.HomeDir (or
equivalent field) while leaving other behavior unchanged.
In `@framework/docker/evstack/chain_builder.go`:
- Around line 154-163: The Chain config built in Build() is missing the HomeDir
field so any override passed via b.homeDir isn't propagated; update the Config
literal assigned to cfg in Build() (the Config used to construct Chain.cfg) to
include HomeDir: b.homeDir so that Chain.cfg.HomeDir is set, keeping newNode()
unchanged (it already receives b.homeDir). Ensure you set the HomeDir field on
the Config struct used when creating the Chain so downstream code reading
Chain.cfg.HomeDir sees the override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74d55a87-29de-4f31-9b38-092d303d9b47
📒 Files selected for processing (21)
framework/docker/container/job.goframework/docker/cosmos/chain_builder.goframework/docker/da_network_test.goframework/docker/dataavailability/config.goframework/docker/dataavailability/network.goframework/docker/dataavailability/network_builder.goframework/docker/dataavailability/node.goframework/docker/evstack/chain_builder.goframework/docker/evstack/config.goframework/docker/evstack/evmsingle/builder.goframework/docker/evstack/evmsingle/config.goframework/docker/evstack/evmsingle/node.goframework/docker/evstack/node.goframework/docker/evstack/reth/builder.goframework/docker/evstack/reth/config.goframework/docker/evstack/reth/node.goframework/docker/evstack/spamoor/builder.goframework/docker/evstack/spamoor/node.goframework/docker/ibc/relayer/hermes_types.goframework/docker/internal/keyring.goframework/docker/reth_test.go
✅ Files skipped from review due to trivial changes (5)
- framework/docker/reth_test.go
- framework/docker/container/job.go
- framework/docker/ibc/relayer/hermes_types.go
- framework/docker/internal/keyring.go
- framework/docker/da_network_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- framework/docker/evstack/evmsingle/node.go
- framework/docker/evstack/reth/builder.go
- framework/docker/evstack/reth/node.go
- framework/docker/evstack/spamoor/node.go
- framework/docker/evstack/evmsingle/config.go
- framework/docker/evstack/evmsingle/builder.go
- framework/docker/evstack/spamoor/builder.go
- framework/docker/evstack/reth/config.go
- framework/docker/evstack/node.go
- framework/docker/cosmos/chain_builder.go
Volume ownership was set via UIDGID but the container runtime user was not, causing containers to run as root while volumes were owned by a different UID. Setting User in container.Config ensures the runtime user matches the volume owner.
Setting User to empty string overrides the image USER directive and forces the container to run as root. Only set it when UIDGID is non-empty to preserve the image default.
Pass image UIDGID as User in Node.Exec so init commands run as the same user as the main container, preventing file ownership mismatches. Remove UIDGID from ibc-go-simd test image since simd writes to $HOME/.simapp on startup which fails as non-root.
Binary writes to /app/storage/ outside the volume mount, requires root.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
ref evstack/ev-node#3082
Summary by CodeRabbit
New Features
Chores
Bug Fixes