Add bootnode template, allow configuring base recipe flags through yaml.#347
Add bootnode template, allow configuring base recipe flags through yaml.#347canercidam merged 2 commits intomainfrom
Conversation
Enable YAML recipes to reference the bootnode enode URL using {{Bootnode}}
template. This allows custom services to connect via p2p to the bootnode
without hardcoding addresses.
Allow YAML recipes to pass arguments to the base recipe's flags. This enables setting options like --enable-latest-fork for opstack.
|
Claude finished @dvush's task in 2m 9s —— View job PR Review CompleteReviewed the changes adding SummaryThe core functionality works correctly for opstack-based YAML recipes. The Issues Found
See inline comments for details. |
| "Bootnode": func() string { | ||
| if d.manifest.Bootnode == nil { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
This silently returns an empty string when Bootnode is nil (e.g., when using base: l1). This could lead to confusing behavior where commands have empty values where the enode URL was expected. Consider logging a warning or returning an error indicator so users know something is wrong.
| return "" | ||
| } | ||
| svc := d.manifest.MustGetService(d.manifest.Bootnode.Service) | ||
| port := svc.MustGetPort("rpc") |
There was a problem hiding this comment.
Hard-coded port name "rpc" assumes the bootnode service always has this port. If a custom YAML recipe uses a different port name, this will panic via MustGetPort. Consider making the port name part of BootnodeRef or documenting this requirement.
Here are the changes to make my custom recipes to work.
Enable YAML recipes to reference the bootnode enode URL using {{Bootnode}}
template. This allows custom services to connect via p2p to the bootnode
without hardcoding addresses.
Allow YAML recipes to pass arguments to the base recipe's flags.
This enables setting options like --enable-latest-fork for opstack.
The second one allow doing this in custom recipes:
I am not sure if its the best approach for this but it works, there should be a way to configure it in some way