Skip to content

refactor: add NodeBuilder to simplify container.Node creation #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chatton wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor: add NodeBuilder to simplify container.Node creation #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb27f33
refactor: add NodeBuilder to simplify container.Node creation (#131)
chatton May 21, 2026
430c88e
Merge remote-tracking branch 'origin/main' into chatton/simplify-cont…
chatton May 21, 2026
9996951
fix: correct nodeType derivation and harden docker keyring deletion
chatton May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions framework/docker/chain_node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestChainNodeHostName(t *testing.T) {
AdditionalStartArgs: []string{},
EncodingConfig: &testutil.TestEncodingConfig{},
}
node1 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 0, chainParams1)
node1 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 0, chainParams1) //nolint:staticcheck // testing deprecated API

chainParams2 := cosmos.ChainNodeParams{
Validator: true,
Expand All @@ -43,7 +43,7 @@ func TestChainNodeHostName(t *testing.T) {
AdditionalStartArgs: []string{},
EncodingConfig: &testutil.TestEncodingConfig{},
}
node2 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 1, chainParams2)
node2 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 1, chainParams2) //nolint:staticcheck // testing deprecated API

chainParams3 := cosmos.ChainNodeParams{
Validator: false,
Expand All @@ -56,7 +56,7 @@ func TestChainNodeHostName(t *testing.T) {
AdditionalStartArgs: []string{},
EncodingConfig: &testutil.TestEncodingConfig{},
}
node3 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 2, chainParams3)
node3 := cosmos.NewChainNode(logger, &tastoraclient.Client{}, "test-network", testName, container.Image{}, "/test/home", 2, chainParams3) //nolint:staticcheck // testing deprecated API

// get hostnames
hostname1 := node1.HostName()
Expand Down
82 changes: 82 additions & 0 deletions framework/docker/container/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type Node struct {
Logger *zap.Logger
}

// Deprecated: use NewNodeBuilder instead.
// NewNode creates a new Node instance with the required parameters.
func NewNode(
networkID string,
Expand All @@ -53,6 +54,86 @@ func NewNode(
}
}

// NodeBuilder provides a fluent API for constructing and initializing a Node.
type NodeBuilder struct {
networkID string
dockerClient types.TastoraDockerClient
testName string
image Image
homeDir string
index int
nodeType types.NodeType
logger *zap.Logger
volumeName string
hostNetwork bool
}

// NewNodeBuilder creates a NodeBuilder with the minimum required fields.
func NewNodeBuilder(dockerClient types.TastoraDockerClient, testName string, image Image, logger *zap.Logger) *NodeBuilder {
return &NodeBuilder{
dockerClient: dockerClient,
testName: testName,
image: image,
logger: logger,
}
}

func (b *NodeBuilder) WithNetworkID(networkID string) *NodeBuilder {
b.networkID = networkID
return b
}

func (b *NodeBuilder) WithHomeDir(homeDir string) *NodeBuilder {
b.homeDir = homeDir
return b
}

func (b *NodeBuilder) WithIndex(index int) *NodeBuilder {
b.index = index
return b
}

func (b *NodeBuilder) WithNodeType(nodeType types.NodeType) *NodeBuilder {
b.nodeType = nodeType
return b
}

// WithHostNetwork enables host network mode on the container lifecycle.
func (b *NodeBuilder) WithHostNetwork(hostNetwork bool) *NodeBuilder {
b.hostNetwork = hostNetwork
return b
}

// WithVolumeName overrides the name used for volume creation.
// By default, Build uses the containerName for the volume. Use this to share
// a volume with another node (e.g. a deployer and its agent).
func (b *NodeBuilder) WithVolumeName(volumeName string) *NodeBuilder {
b.volumeName = volumeName
return b
}

// Build creates the Node with its lifecycle and volume fully initialized.
// The containerName is used for the lifecycle and (unless WithVolumeName was called)
// for the volume. Callers should pass the name that the embedding type will use
// as its container name.
func (b *NodeBuilder) Build(ctx context.Context, containerName string) (*Node, error) {
n := NewNode(b.networkID, b.dockerClient, b.testName, b.image, b.homeDir, b.index, b.nodeType, b.logger)
lc := NewLifecycle(b.logger, b.dockerClient, containerName)
if b.hostNetwork {
lc.SetHostNetwork(b.hostNetwork)
}
n.ContainerLifecycle = lc
volName := containerName
if b.volumeName != "" {
volName = b.volumeName
}
if err := n.CreateAndSetupVolume(ctx, volName); err != nil {
return nil, fmt.Errorf("setup node %q: %w", containerName, err)
}
return n, nil
}
Comment on lines +119 to +134
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate required builder inputs in Build before creating the node.

Build currently allows invalid state (e.g., empty containerName/homeDir, or empty networkID when host mode is off), which can fail later with less actionable Docker errors. Add upfront validation here to fail fast.

Suggested patch 
 func (b *NodeBuilder) Build(ctx context.Context, containerName string) (*Node, error) {
+	if containerName == "" {
+		return nil, fmt.Errorf("containerName cannot be empty")
+	}
+	if b.homeDir == "" {
+		return nil, fmt.Errorf("homeDir cannot be empty")
+	}
+	if !b.hostNetwork && b.networkID == "" {
+		return nil, fmt.Errorf("networkID cannot be empty when host network is disabled")
+	}
+
 	n := NewNode(b.networkID, b.dockerClient, b.testName, b.image, b.homeDir, b.index, b.nodeType, b.logger)
 	lc := NewLifecycle(b.logger, b.dockerClient, containerName)
 	if b.hostNetwork {
 		lc.SetHostNetwork(b.hostNetwork)
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (b *NodeBuilder) Build(ctx context.Context, containerName string) (*Node, error) {
n := NewNode(b.networkID, b.dockerClient, b.testName, b.image, b.homeDir, b.index, b.nodeType, b.logger)
lc := NewLifecycle(b.logger, b.dockerClient, containerName)
if b.hostNetwork {
lc.SetHostNetwork(b.hostNetwork)
}
n.ContainerLifecycle = lc
volName := containerName
if b.volumeName != "" {
volName = b.volumeName
}
if err := n.CreateAndSetupVolume(ctx, volName); err != nil {
return nil, fmt.Errorf("setup node %q: %w", containerName, err)
}
return n, nil
}
func (b *NodeBuilder) Build(ctx context.Context, containerName string) (*Node, error) {
if containerName == "" {
return nil, fmt.Errorf("containerName cannot be empty")
}
if b.homeDir == "" {
return nil, fmt.Errorf("homeDir cannot be empty")
}
if !b.hostNetwork && b.networkID == "" {
return nil, fmt.Errorf("networkID cannot be empty when host network is disabled")
}
n := NewNode(b.networkID, b.dockerClient, b.testName, b.image, b.homeDir, b.index, b.nodeType, b.logger)
lc := NewLifecycle(b.logger, b.dockerClient, containerName)
if b.hostNetwork {
lc.SetHostNetwork(b.hostNetwork)
}
n.ContainerLifecycle = lc
volName := containerName
if b.volumeName != "" {
volName = b.volumeName
}
if err := n.CreateAndSetupVolume(ctx, volName); err != nil {
return nil, fmt.Errorf("setup node %q: %w", containerName, err)
}
return n, 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/container/node.go` around lines 119 - 134, The Build method
on NodeBuilder should validate required inputs before creating the Node: check
that containerName and b.homeDir are non-empty, and if b.hostNetwork is false
ensure b.networkID is non-empty; return a clear error (with context mentioning
NodeBuilder.Build and containerName) if any check fails. Do these checks at the
top of NodeBuilder.Build (before calling
NewNode/NewLifecycle/CreateAndSetupVolume) so invalid config is rejected fast
and later calls like NewNode or CreateAndSetupVolume are not invoked with
missing values.


// Deprecated: use NewNodeBuilder instead.
// SetContainerLifecycle sets the container lifecycle for the node
func (n *Node) SetContainerLifecycle(lifecycle *Lifecycle) {
n.ContainerLifecycle = lifecycle
Expand Down Expand Up @@ -164,6 +245,7 @@ func (n *Node) GetVolumeName(nodeName string) string {
return internal.SanitizeDockerResourceName(n.TestName + "-" + nodeName + "-vol")
}

// Deprecated: use NewNodeBuilder instead.
// CreateAndSetupVolume creates a Docker volume for the node and sets up proper ownership.
// This consolidates the volume creation pattern used across all node types.
// The nodeName parameter should be the specific name for this node instance.
Expand Down
61 changes: 37 additions & 24 deletions framework/docker/cosmos/chain_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,26 +482,6 @@ func (b *ChainBuilder) newChainNode(
nodeConfig ChainNodeConfig,
index int,
) (*ChainNode, error) {
// Construct the ChainNode first so we can access its name.
// The ChainNode's VolumeName cannot be set until after we create the volume.
tn := b.newDockerChainNode(b.logger, nodeConfig, index)

// create and setup volume using shared logic
if err := tn.CreateAndSetupVolume(ctx, tn.Name()); err != nil {
return nil, err
}

// if this is a validator and we have a genesis keyring, preload the keys using a one-shot container
if nodeConfig.nodeType == types.NodeTypeValidator && tn.GenesisKeyring != nil {
if err := preloadKeyringToVolume(ctx, tn, nodeConfig); err != nil {
return nil, fmt.Errorf("failed to preload keyring to volume: %w", err)
}
}

return tn, nil
}

func (b *ChainBuilder) newDockerChainNode(log *zap.Logger, nodeConfig ChainNodeConfig, index int) *ChainNode {
homeDir := b.homeDir
if homeDir == "" {
homeDir = "/var/cosmos-chain"
Expand All @@ -510,9 +490,15 @@ func (b *ChainBuilder) newDockerChainNode(log *zap.Logger, nodeConfig ChainNodeC
}
}

// default unspecified node type to full node
nodeType := nodeConfig.nodeType
if nodeType == 0 {
nodeType = types.NodeTypeConsensusFull
}

chainParams := ChainNodeParams{
Validator: nodeConfig.nodeType == types.NodeTypeValidator,
NodeType: nodeConfig.nodeType,
Validator: nodeType == types.NodeTypeValidator,
NodeType: nodeType,
ChainID: b.chainID,
BinaryName: b.binaryName,
CoinType: b.coinType,
Expand All @@ -528,10 +514,37 @@ func (b *ChainBuilder) newDockerChainNode(log *zap.Logger, nodeConfig ChainNodeC
AdditionalExposedPorts: b.additionalExposedPorts,
}

// Get the appropriate image using fallback logic
imageToUse := b.getImage(nodeConfig)

return NewChainNode(log, b.dockerClient, b.dockerNetworkID, b.testName, imageToUse, homeDir, index, chainParams)
log := b.logger.With(
zap.Bool("validator", chainParams.Validator),
zap.Int("i", index),
)

name := chainNodeName(b.testName, index, b.chainID, nodeType)
baseNode, err := container.NewNodeBuilder(b.dockerClient, b.testName, imageToUse, log).
WithNetworkID(b.dockerNetworkID).
WithHomeDir(homeDir).
WithIndex(index).
WithNodeType(nodeType).
Build(ctx, name)
if err != nil {
return nil, err
}

tn := &ChainNode{
ChainNodeParams: chainParams,
Node: baseNode,
}

// if this is a validator and we have a genesis keyring, preload the keys using a one-shot container
if nodeConfig.nodeType == types.NodeTypeValidator && tn.GenesisKeyring != nil {
if err := preloadKeyringToVolume(ctx, tn, nodeConfig); err != nil {
return nil, fmt.Errorf("failed to preload keyring to volume: %w", err)
}
}

return tn, nil
}

// preloadKeyringToVolume copies validator keys from genesis keyring to the node's volume
Expand Down
7 changes: 6 additions & 1 deletion framework/docker/cosmos/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ type ChainNodeParams struct {
AdditionalExposedPorts []string
}

// Deprecated: use container.NewNodeBuilder instead.
// NewChainNode creates a new ChainNode with injected dependencies
func NewChainNode(
logger *zap.Logger,
Expand Down Expand Up @@ -248,9 +249,13 @@ func (cn *ChainNode) GetKeyring() (keyring.Keyring, error) {
return internal.NewDockerKeyring(cn.DockerClient, cn.ContainerLifecycle.ContainerID(), containerKeyringDir, cn.EncodingConfig.Codec), nil
}

func chainNodeName(testName string, index int, chainID string, nodeType types.ConsensusNodeType) string {
return fmt.Sprintf("%s-%s-%d-%s", chainID, nodeType.String(), index, internal.SanitizeDockerResourceName(testName))
}

// Name of the test node container.
func (cn *ChainNode) Name() string {
return fmt.Sprintf("%s-%s-%d-%s", cn.ChainID, cn.NodeType(), cn.Index, internal.SanitizeDockerResourceName(cn.TestName))
return chainNodeName(cn.TestName, cn.Index, cn.ChainID, cn.ChainNodeParams.NodeType)
}

// NodeType returns the type of the ChainNode as a string.
Expand Down
25 changes: 22 additions & 3 deletions framework/docker/dataavailability/network_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,13 +237,32 @@ func (b *NetworkBuilder) newNode(ctx context.Context, nodeConfig NodeConfig, ind
nodeConfig.Env = b.env
}

node := NewNode(cfg, b.testName, imageToUse, index, nodeConfig)
homeDir := cfg.HomeDir
if homeDir == "" {
homeDir = DefaultHomeDir()
}

// Create and setup volume using shared logic
if err := node.CreateAndSetupVolume(ctx, node.Name()); err != nil {
log := b.logger.With(zap.String("node_type", nodeConfig.NodeType.String()))
name := daNodeName(b.testName, index, nodeConfig.NodeType)
baseNode, err := container.NewNodeBuilder(b.dockerClient, b.testName, imageToUse, log).
WithNetworkID(b.dockerNetworkID).
WithHomeDir(homeDir).
WithIndex(index).
WithNodeType(nodeConfig.NodeType).
Build(ctx, name)
if err != nil {
return nil, err
}

node := &Node{
cfg: cfg,
nodeType: nodeConfig.NodeType,
additionalStartArgs: nodeConfig.AdditionalStartArgs,
configModifications: nodeConfig.ConfigModifications,
internalPorts: initializeDANodePorts(nodeConfig.InternalPorts),
Node: baseNode,
}

// Run post-init functions if any
for _, postInitFn := range nodeConfig.postInit {
if err := postInitFn(ctx, node); err != nil {
Expand Down
7 changes: 6 additions & 1 deletion framework/docker/dataavailability/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ type Node struct {
externalPorts types.Ports
}

// Deprecated: use container.NewNodeBuilder instead.
func NewNode(cfg Config, testName string, image container.Image, index int, nodeConfig NodeConfig) *Node {
homeDir := cfg.HomeDir
if homeDir == "" {
Expand All @@ -97,9 +98,13 @@ func NewNode(cfg Config, testName string, image container.Image, index int, node
return node
}

func daNodeName(testName string, index int, nodeType types.DANodeType) string {
return fmt.Sprintf("da-%s-%d-%s", nodeType.String(), index, internal.SanitizeDockerResourceName(testName))
}

// Name returns the container name for the Node.
func (n *Node) Name() string {
return fmt.Sprintf("da-%s-%d-%s", n.nodeType.String(), n.Index, internal.SanitizeDockerResourceName(n.TestName))
return daNodeName(n.TestName, n.Index, n.nodeType)
}

// HostName returns the condensed hostname for the Node.
Expand Down
27 changes: 24 additions & 3 deletions framework/docker/evstack/chain_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,34 @@ func (b *ChainBuilder) newNode(ctx context.Context, nodeConfig NodeConfig, index
HomeDir: b.homeDir,
}

node := NewNode(cfg, b.testName, imageToUse, index, nodeConfig.IsAggregator, b.getAdditionalStartArgs(nodeConfig))
homeDir := cfg.HomeDir
if homeDir == "" {
homeDir = DefaultHomeDir()
}

// Create and setup volume using shared logic
if err := node.CreateAndSetupVolume(ctx, node.Name()); err != nil {
log := b.logger.With(
zap.Int("i", index),
zap.Bool("aggregator", nodeConfig.IsAggregator),
)

name := evstackNodeName(b.testName, index, b.chainID)
baseNode, err := container.NewNodeBuilder(b.dockerClient, b.testName, imageToUse, log).
WithNetworkID(b.dockerNetworkID).
WithHomeDir(homeDir).
WithIndex(index).
WithNodeType(EvstackType).
Build(ctx, name)
if err != nil {
return nil, err
}

node := &Node{
cfg: cfg,
isAggregatorFlag: nodeConfig.IsAggregator,
additionalStartArgs: b.getAdditionalStartArgs(nodeConfig),
Node: baseNode,
}

// Run post-init functions if any
for _, postInitFn := range nodeConfig.postInit {
if err := postInitFn(ctx, node); err != nil {
Expand Down
26 changes: 18 additions & 8 deletions framework/docker/evstack/evmsingle/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,31 @@ func newNode(ctx context.Context, cfg Config, testName string, index int, nodeCf

log := cfg.Logger.With(zap.String("component", "evm-single"), zap.Int("i", index))

n := &Node{cfg: cfg, nodeCfg: nodeCfg, logger: log, internal: ports, chainName: chainName}
n.Node = container.NewNode(cfg.DockerNetworkID, cfg.DockerClient, testName, image, cfg.HomeDir, index, NodeType, log)
n.SetContainerLifecycle(container.NewLifecycle(cfg.Logger, cfg.DockerClient, n.Name()))
if err := n.CreateAndSetupVolume(ctx, n.Name()); err != nil {
containerName := evmSingleNodeName(testName, index, chainName)
node, err := container.NewNodeBuilder(cfg.DockerClient, testName, image, log).
WithNetworkID(cfg.DockerNetworkID).
WithHomeDir(cfg.HomeDir).
WithIndex(index).
WithNodeType(NodeType).
Build(ctx, containerName)
if err != nil {
return nil, err
}
n := &Node{cfg: cfg, nodeCfg: nodeCfg, logger: log, internal: ports, chainName: chainName}
n.Node = node
return n, nil
}

func evmSingleNodeName(testName string, index int, chainName string) string {
if chainName != "" {
return fmt.Sprintf("evm-single-%s-%d-%s", chainName, index, internal.SanitizeDockerResourceName(testName))
}
return fmt.Sprintf("evm-single-%d-%s", index, internal.SanitizeDockerResourceName(testName))
}

// Name returns a stable container name
func (n *Node) Name() string {
if n.chainName != "" {
return fmt.Sprintf("evm-single-%s-%d-%s", n.chainName, n.Index, internal.SanitizeDockerResourceName(n.TestName))
}
return fmt.Sprintf("evm-single-%d-%s", n.Index, internal.SanitizeDockerResourceName(n.TestName))
return evmSingleNodeName(n.TestName, n.Index, n.chainName)
}

// HostName returns a condensed hostname
Expand Down
7 changes: 6 additions & 1 deletion framework/docker/evstack/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type Node struct {
externalPorts types.Ports
}

// Deprecated: use container.NewNodeBuilder instead.
func NewNode(cfg Config, testName string, image container.Image, index int, isAggregator bool, additionalStartArgs []string) *Node {
homeDir := cfg.HomeDir
if homeDir == "" {
Expand All @@ -82,9 +83,13 @@ func NewNode(cfg Config, testName string, image container.Image, index int, isAg
return node
}

func evstackNodeName(testName string, index int, chainID string) string {
return fmt.Sprintf("%s-evstack-%d-%s", chainID, index, internal.SanitizeDockerResourceName(testName))
}

// Name of the test node container.
func (n *Node) Name() string {
return fmt.Sprintf("%s-evstack-%d-%s", n.cfg.ChainID, n.Index, internal.SanitizeDockerResourceName(n.TestName))
return evstackNodeName(n.TestName, n.Index, n.cfg.ChainID)
}

// HostName returns the condensed hostname for the Node.
Expand Down
Loading
Loading