feat: add --temporalAddress option#2175
Conversation
…ralNamespace in globalOptions from serverOptions. Homogenize the option name as temporalAddress across the platform
lschim
force-pushed
the
feat/temporal-cli-options
branch
from
575861c to
aafe87e
Compare
pirhoo
changed the title
pirhoo
left a comment
pirhoo
left a comment
There was a problem hiding this comment.
Only two small suggestions but it's good to go!
| QueueType batchQueueType; | ||
|
|
||
| @Option(names = {"--temporalAddress"}, description = "Temporal address", scope = ScopeType.INHERIT) | ||
| String temporalAddress = EnvUtils.resolveUri("temporal", "temporal:7233"); |
There was a problem hiding this comment.
hint: I guess that means temporalAddress can never be empty and the putIfNotNull line 160 might shadow the default value.
There was a problem hiding this comment.
After review with Pierre, it was determined that it would stay that way because the picocli default value in the annotation needs to be a constant, meaning we won't be able to get the value from environment variables or conf files. Also, all the other address options were written exactly the same way
| List.of(TEMPORAL_ADDRESS_OPT), "Temporal address") | ||
| .withRequiredArg() | ||
| .ofType( String.class ) | ||
| .defaultsTo("temporal:7233"); |
There was a problem hiding this comment.
suggestion: this breaks the file convention to store default value in DEFAULT_* constants. We might do some girl-scouting here and apply it to other default values.
There was a problem hiding this comment.
Noted, the file was girl-scouted to use DEFAULT_* constants !
2 participants
Added in picocli as a globalOption for it to be available in SERVER and WORKER commands.
Added in legacy CLI
Homogenize the option across the platform