Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
lorensr opened this issue Aug 3, 2023 · 5 comments
Open
Labels
enhancement New feature or request

Comments

@lorensr
Copy link
Contributor

lorensr commented Aug 3, 2023

repetitive and doesn't fit pattern:

@lorensr lorensr added the enhancement New feature or request label Aug 3, 2023
@cretz
Copy link
Member

cretz commented Aug 3, 2023

This was intentionally named --dynamic-config-value in temporalio/temporalite-archived#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)).

@lorensr
Copy link
Contributor Author

lorensr commented Aug 4, 2023

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.

@feedmeapples
Copy link
Contributor

there was some discussion that the dynamic config can be part of the unified Server & UI config #184

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

@cretz
Copy link
Member

cretz commented Aug 4, 2023

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.

@lorensr
Copy link
Contributor Author

lorensr commented Aug 6, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants