fix: wakunode2 config. adding new 'topic' config parameter. #1727
Conversation
This new parameter can be repeated and we are starting to deprecate the parameter 'topics', that expected to receive a space-separated list of pubsubtopic to subscribe to.
apps/wakunode2/app.nim
Outdated
| let pubsubTopics = conf.topics.split(" ") | ||
|
|
||
| var pubsubTopics = @[""] | ||
| if conf.topicsDeprecated != "": |
There was a problem hiding this comment.
How about logging a warning if conf.topicsDeprecated != "" ?
There was a problem hiding this comment.
Good point! I'll add it
There was a problem hiding this comment.
agree.
if conf.topicsDeprecated != "" and conf.topics.len !=0.
error
if conf.topicsDeprecated != "":
warn
There was a problem hiding this comment.
Now, if topicsDeprecated is given, it prints a warn. If both topicsDeprecated and topics are given, then an error is returned and the execution is halted.
There was a problem hiding this comment.
thanks for this, left a comment which i think its important, otherwise we will be introducing breaking changes.
and out of curiosity, why from file its like this? i find it confusing that the behaviour is different. cant we have the same? topic=a, topic=b?
topic = [ "t1" "t2" "t3" ]
mind having a look on how staticnode(s) is implemented?
apps/wakunode2/config.nim
Outdated
| defaultValue: "/waku/2/default-waku/proto" | ||
| topicsDeprecated* {. | ||
| desc: "Default topics to subscribe to (space separated list). DEPRECATED: please use repeated --topic argument instead." | ||
| defaultValue: "" |
There was a problem hiding this comment.
should we keep the /waku/2/default-waku/proto here? otherwise old deployments using "topics" will break? (meaning they will no longer be subscribed)
There was a problem hiding this comment.
Oh! Important point that I missed. @alrevuelta is right here. I think we should keep the default in both config options.
There was a problem hiding this comment.
It shouldn't break because if nothing is specified, the default pubsub topic will get used.
There was a problem hiding this comment.
you are right:
Line 559 in 493f082
but perhaps a bit convoluted? can see you applied the changes thanks!
This behavior comes from 'confutils'. It behaves equally for any other repeatable parameter, i.e. I absolutely agree that it is confusing. |
This change is interesting due to the deprecation of the 'topics' parameter. waku-org/nwaku#1727
Description
Adding a new
topicparameter can be repeated.The
topicsparameter becomes deprecated because it is not consistent with the other "repeatable" ones as it expects a space-separated list of pubsub topics.How to use the new parameter
When setting different pubsub topics through the CLI, they can be repeated:
./build/wakunode2 --topic=t1 --topic=t2 --topic=t3 ...However, when setting them from within a config file, they should be in an array format within the cfg file:
Issue
closes #1726