cli icon indicating copy to clipboard operation
cli copied to clipboard

[Feature Request] Rename --dynamic-config-value ➡️ --dynamic-config

Open lorensr opened this issue 2 years ago • 5 comments

repetitive and doesn't fit pattern:

lorensr avatar Aug 03 '23 06:08 lorensr

This was intentionally named --dynamic-config-value in https://github.com/temporalio/temporalite/pull/136 because there is already a server concept called "dynamic config" which is a bunch of dynamic config values, and there is also a concept of a dynamic config file which contains a dynamic config. This is for setting a single dynamic config value, not for setting dynamic config (maybe we will want setting full dynamic config one day, though easily done via config (which is via dynamic config file)).

cretz avatar Aug 03 '23 12:08 cretz

After reading the docs:

https://docs.temporal.io/references/dynamic-configuration

I don't see the issue with calling an individual key/value pair a "dynamic config". I think you're saying the server code calls the set of pairs a dynamic config, and a set of things is different from an individual thing? To me it seems like most CLI users won't know about that distinction, and that it's not important for them to know?

Pro adding an option for dynamic config file, since I think you can't use constraints with the current option? Would use --dynamic-config-file <path> for that.

lorensr avatar Aug 04 '23 01:08 lorensr

there was some discussion that the dynamic config can be part of the unified Server & UI config https://github.com/temporalio/cli/issues/184

If so we will not need a separate flag for dynamic config file. What do you think of this?

feedmeapples avatar Aug 04 '23 06:08 feedmeapples

I think for setting a value using dynamic-config-value makes sense and setting a file dynamic-config-file makes sense and in general setting a dynamic config <something> using dynamic-config-<something> makes sense. But it's not a strongly held opinion.

What is more strongly held is that this is deployed software. You can't just go changing CLI options of this software IMO. SDK team already relies on this in multiple repositories, who knows how many others do. And even if you feel like you can change CLI options of this software, we should require a better justification for backwards incompatible alterations.

cretz avatar Aug 04 '23 11:08 cretz

Is it easy to keep the current option working and hide it from help output? If not, or if others don't think it's an improvement, okay to close!

lorensr avatar Aug 06 '23 23:08 lorensr

Closing this. IMO we should maintain the current behavior:

temporal server start-dev --dynamic-config-value foo=true --dynamic-config-value bar=false

Reasons:

  • It has been this way for some time in temporalite, and in unreleased versions of temporal server start-dev, and hence changing would break many users.
  • It makes some sense: "value" in the sense that "I am setting one key-value pair". To set multiple key-value pairs, you use the option multiple times. --dynamic-config would be better fitted to an API where multiple key-value pairs can be set by supplying the option once.

There's no argument strong enough to justify making the breaking change proposed by this ticket.

dandavison avatar Jun 01 '24 14:06 dandavison