refactor: update to new evnode flag#112
refactor: update to new evnode flag#112chatton merged 2 commits intocelestiaorg:mainfrom julienrbrt:julien/evnode-changes
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughUpdated the CLI flag for the signer path from --evnode.signer.path to --evnode.signer.signer_path in evstack aggregator node setup, adjusting both initialization and container start command construction. No other logic or control flow was changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding ✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
framework/docker/evstack/node.go (1)
105-112: Deduplicate aggregator flags (single helper to avoid future drift)Use one helper so future flag renames happen in one place.
Apply:
@@ func (n *Node) Init(ctx context.Context, initArguments ...string) error { @@ - if n.isAggregator() { - signerPath := filepath.Join(n.HomeDir(), "config") - cmd = append(cmd, - "--evnode.node.aggregator", - "--evnode.signer.passphrase="+n.cfg.AggregatorPassphrase, //nolint:gosec // used for testing only - "--evnode.signer.signer_path="+signerPath) - } + if n.isAggregator() { + cmd = append(cmd, n.aggregatorFlags()...) + } @@ func (n *Node) createEvstackContainer(ctx context.Context, additionalStartArgs ...string) error { @@ - if n.isAggregator() { - signerPath := filepath.Join(n.HomeDir(), "config") - startCmd = append(startCmd, - "--evnode.node.aggregator", - "--evnode.signer.passphrase="+n.cfg.AggregatorPassphrase, //nolint:gosec // used for testing only - "--evnode.signer.signer_path="+signerPath) - } + if n.isAggregator() { + startCmd = append(startCmd, n.aggregatorFlags()...) + }Add nearby:
func (n *Node) aggregatorFlags() []string { signerPath := filepath.Join(n.HomeDir(), "config") return []string{ "--evnode.node.aggregator", "--evnode.signer.passphrase=" + n.cfg.AggregatorPassphrase, //nolint:gosec // tests only "--evnode.signer.signer_path=" + signerPath, } }Also applies to: 151-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
framework/docker/evstack/node.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
framework/docker/evstack/node.go (3)
framework/docker/evstack/node_config.go (2)
IsAggregator(10-21)b(40-43)framework/docker/evstack_test.go (1)
TestEvstack(18-135)framework/docker/evstack/chain_builder.go (3)
b(87-90)t(15-40)b(111-114)
🔇 Additional comments (2)
framework/docker/evstack/node.go (2)
155-156: Start path updated consistentlyStart command now uses --evnode.signer.signer_path as well; consistent with upstream constants/docs. (github.com)
111-112: Confirm CI ev-node image includes PR #2637
No occurrences of the old--evnode.signer.pathflag remain in Go code; please verify that your CI (workflows, Dockerfiles, Jenkinsfiles, etc.) pulls anevstack/ev-nodeimage at or after PR evstack/ev-node#2637, or tests may fail on the renamed flag.
|
@julienrbrt there will be a bunch of breaking changes in the next tag, not too many I think that will just effect evnode but either I can help implement them or you can use the tests in this repo as a reference. Mainly around the construction of the evnodes themselves, it's using the builder pattern now to be consistent with how things work in celestia-app |
|
@julienrbrt it looks like tests still failing, need to check which image is being used |
It just got merged on main. It should be correct now :) |
|
@julienrbrt looks good now 🚀 |
ref: evstack/ev-node#2637
Summary by CodeRabbit