chore: Simplify configuration for the waku network #2404
Conversation
|
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
|
You can find the image built from this PR at Built from f4b8923 |
| @@ -256,6 +258,11 @@ type | |||
| desc: "Default pubsub topic to subscribe to. Argument may be repeated." | |||
| name: "pubsub-topic" .}: seq[string] | |||
|
|
|||
| shards* {. | |||
There was a problem hiding this comment.
Thinking that in future the ability to configure ranges may be needed for large subscriptions (e.g. --shards:0..7,15,21..29). For now repeated arg makes sense.
There was a problem hiding this comment.
mm i would say node operators don't necessarily need to specify the exact shards but the number of them they want to run. unless they are interested in a specific shard because their content topic falls in there, but in that case i expect them to provide the contentTopic and not the shard.
(but yeap, current behaviour is --shard=5 means shard 5 and not 5 shards)
| # cluster-id=1 | ||
| # Cluster configuration corresponding to The Waku Network. Note that it | ||
| # overrides existing cli configuration | ||
| proc TheWakuNetworkConf*(T: type ClusterConf): ClusterConf = |
There was a problem hiding this comment.
Super nitpick, I know xD but comments tend to go below proc definition
|
|
||
| proc parseCmdArg*(T: type ShardIdx, p: string): T = | ||
| try: | ||
| ShardIdx(parseInt(p)) |
There was a problem hiding this comment.
Would it be possible to ensure that the given index is within the valid boundaries?
There was a problem hiding this comment.
will address this in followup prs. don't want to just add a MaxAmountShards here but enforce it all over the place when a given cluster-id is chosen. and that may require some changes out of scope of this pr :)
| conf.discv5BootstrapNodes = | ||
| conf.discv5BootstrapNodes & twnClusterConf.discv5BootstrapNodes |
There was a problem hiding this comment.
Is this overriding the configuration or just appending made on purpose?
There was a problem hiding this comment.
appending, just in case someone wants to add more bootstrap nodes on top of the existing ones.
Description
Simplifies configuration, extracting
cluster-id=1parameters into a file. Whencluster-id=1is configured, then all parameters are loaded without having to specify them all. Now someone wanting to joinTheWakuNetworkcan do so with just:Or if you want to run just one shard(s):
This should simplify this monster.