External client configuration proposal #94
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,22 +24,33 @@ What are the reasons for building this? | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### Goals | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| * Common, well-specified/type-safe configuration format usable by Temporal tooling for client options and usable | ||||||||||||||||||||||||||||||||||||||||||||||||
| manually by advanced users | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Implementation in all Temporal SDKs for reading and writing of this configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
| * All Temporal CLI/SDKs updated to have easy client creation using this configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Common on-disk location in every platform for reading/writing this format | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Ability to provide environment variables to represent values in the configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Well-specified hierarchy for on-disk overridable by environment variables overridable by in-CLI/SDK values | ||||||||||||||||||||||||||||||||||||||||||||||||
| * 0 new dependencies in SDKs | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. This is broad and seems like it precludes using any prebuilt configuration packages, which seems like it should probably be part of the discussion. What are you really trying to avoid, how critical is that (P0/P1/P2), and why? There was a problem hiding this comment.
Trying to avoid any new dependencies in our libraries that then forces uses to depend on them transitively and locks them into our version constraints. We have seen this issue with the libraries we do have to depend on (e.g. protobuf) where by using our libraries, we are forcing our users' apps to be bound by version constraints we have. Libraries like ours should avoid dependencies they don't need because they are transitively infectious to a user's entire application.
Yes, and this is supported in all SDKs. In Python it's just an "extra" you have to enable, but for all others these extensions with extra dependencies are entirely separate libraries they have to depend on. The tradeoff is that they now have to add a separate dependency just for configuration support which I was hoping to avoid. Ideally this is as easy as possible for users to consume, and adding a separate library for them to depend on (or force an extra upon install) is not as easy as not having to do it. To compare with AWS SDK, their configuration format they write parsers for in every language, but we don't have to go that far. I will add some clarity to the document explaining this. I think the burden is on justifying the addition of a dependency, not maintaining the lack of one. EDIT: Added clarity on why 0 dependencies and added open question for using extensions instead. There was a problem hiding this comment. I do not want optional "you can use this configuration format" extensions; I think that would be an unnecessary scope expansion and not good value for the complexity required. As much as I hate JSON as a config format, I have to agree with Chad that avoiding new dependencies, and avoiding optional extensions for something this small, is probably the correct call so long as we continue to support people who are opinionated enough to roll their own. There was a problem hiding this comment. TOML is becoming very popular among respected open source projects for configuration (e.g. Python and Rust have both chosen it). To make sure we've considered all possibilities, let's add vendoring TOML parsers as something to consider. We'd have to keep up with any security patches, but otherwise these should be stable. The most important thing here is DX when using the config files. And we've got to assume that new settings will be added to these config files over time, as we add new product features. As mentioned elsewhere in the proposal, the lack of comments in JSON is extremely undesirable. And of course, the comma-requirements result in very bad DX for anyone who is editing directly and not using an autoformatting editor-extension for JSON. There was a problem hiding this comment. 👍 Vendoring is an option. Another challenge is type-safe contracts. With JSON we have a form of type-safe contract across every SDK - protobuf. It supports nesting, documentation, merging, serialization/deserialization, unhandled field support, requires 0 new dependencies, and there's a precedent w/ our usage of it in similar ways (e.g. workflow history). With TOML, the models will have to be separately maintained in every language. Obviously totally doable, just a series of tradeoffs. Also, I think it's worth discussing how much we expect configuration to be hand-edited. I don't think many people hand-edit AWS SDK/CLI configuration (I have admittedly been using them as a reference here because there's not a lot of SDK configurations out there). Why do we think more people will hand-edit our SDK/CLI config vs that SDK/CLI config? Are we making blind assumptions here? Also, I do know that vscode and other similar tools have no problem with hand-editing JSON (with comments), is their hand-edited configuration story unacceptable for us? There was a problem hiding this comment.
IMO JSON works as a choice for VSCode because they wrapped the parser to allow comments and tolerate trailing commas, which we could do, but that's more technical hassle than vendoring a TOML parser. (Also in VSCode they know that many users will have an autoformatter extension such as There was a problem hiding this comment.
I am not sure this is true. Remember, vendoring includes throwing our our ability to have protobuf model. But still, I would be ok w/ JSON w/ no comments. There was a problem hiding this comment.
This a good flag--let's state that as the goal rather than the implementation choice of not taking any dependencies at all. For example, we could rely on older versions of certain technologies so we're not forcing people to upgrade. Or we could provide a fallback plan so that most users get nice things. Also, I also if, in some languages, we can make dependencies dynamic and only loaded, for example, when a certain file is present. +1 that the ability to comment is worth listing among our goals and deciding whether it's a goal. There was a problem hiding this comment.
No, version constraints go in all directions for compatibility because a newer version may break compatibility in a major version. There is no such thing as "depending on every version ever into the future", you have to have some constraint. If we say "I depend on toml 1.x", when "toml 2.x" comes about our users come complaining to us saying we're holding them back from upgrading. And you can't say "toml x.x" because newer ones may break compat so we have to pick one. And you can't just upgrade to "toml 2.x" when released because many users don't want to upgrade. These are not hypothetical, we've seen all of these complaints and situations in practice. This is why Dan mentioned vendoring (or "shading" if you're a JVM person (kinda)...this has its own issues but still doable, granted it may not have enough value IMO). It is important for us not to impose constraints on our users unless we absolutely have to. I suppose I can change "0 dependencies" to "0 external library version constraints imposed on our users" if the former is unacceptable as a stated goal, but they are saying the same things.
I don't think it is a goal, but that's where this discussion comes in, because if what I think are the limited goals mismatch with what other people think the goals must be, we can reach consensus/decision. |
||||||||||||||||||||||||||||||||||||||||||||||||
| * 💭 Why? | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Dependencies in our SDKs subject our users to these dependencies and their version constraints, so we should avoid | ||||||||||||||||||||||||||||||||||||||||||||||||
| this as much as possible. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * ❓ Can we just have an "extension" that has new dependencies? | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Yes, but ideally users don't have to add dependencies on extension libraries just to get this functionality. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Profile names within the configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Why do we need this? Can we not achieve the same thing in a simpler fashion by providing a way to set the configuration file path from the environment (for example)? The only value I can think of for having them is in the CLI itself, where it's convenient to be able to refer to different environments by short names rather than full config file paths. But I'm not sure if that use case needs to bleed into the SDKs. Your example below of downloading a premade config from the cloud illustrates this point—profiles don't help you here, because your config is in a separate file anyway, and you'd have to go thru extra steps to merge the downloaded config with your local config if you wanted to use it as a profile. There was a problem hiding this comment.
Multiple profiles in the same configuration makes it easier to edit from CLI and have a single config to ship around with multiple profiles if you want.
We offer this too, but the default is a single place.
CLI and SDKs need to share the same approach here. A single config file that contains multiple profiles helps all clients IMO.
That they don't help you there doesn't mean they don't help you anywhere. That's not a multi-profile use case. Having every profile be a different file I think is a bit rough for multi-profile use cases and sharing of multi-profile configuration. EDIT: I added a clarification on why multi-profile-single-file. There was a problem hiding this comment.
Why? The CLI has some use cases/scenarios that don't apply to the (other) SDKs, like being used for operational/admin tasks, possibly in multiple environments.
I am struggling to come up with a "multi-profile use case" that can't also easily be handled with multi-file. Can you provide an example? There was a problem hiding this comment.
Because the same configuration needs to work in all Temporal clients (SDKs and CLIs) consistently.
Sure, and Java has scenarios that don't apply to Python and so on. But this configuration should apply to both. I think AWS CLI vs SDK is a decent model here. I think consistency should be the default and we need a really good reason to be inconsistent.
Sure, technically everything you can do with a single file you can do with multi file, so you will struggle if you're trying to see a situation where we must force multi file. Same goes with any config that has separate sections. For instance, you could say you're struggling to come up with a use case why the The reason we have nested keys in a configuration is for convenience of not having to ship around multiple files or deal with multiple files. You can load up an entire file at once and mutate multiple profiles. You can onboard a new dev by giving a single file with all profiles in it pointing to different clusters. It is easier to glance at all configuration in a file. The entire configuration for use in an SDK can be one set of bytes (some users may not use files, they may provide the bytes some other way). You can load configuration for all profiles from a single database or S3 blob. Etc. The same reasons you may want more than one profile in a file extend to wanting more than one key in a file. I think justifying not using a separate file for separate keys (whether those keys are profiles or just nested options like I think you're a bit too hung up on files here, files are just one way bytes are stored. Configuration is loaded from bytes. There was a problem hiding this comment. Let's classify the contexts in which the config file will be used into deployment contexts (whether prod or staging environments) vs development contexts. In deployment contexts, a single file with a single profile will probably be what's wanted, with client connection secrets injected via env vars. Regarding development contexts, consider an organization with a large monorepo containing many different projects that make use of Temporal. The different projects in the monorepo will want the ability to commit independent Temporal config files in their own subdirectories of the monorepo. In this context, the single So in such a codebase, multiple config files is an inevitable reality. Some questions are:
Open-source projects such as git and uv have chosen:
Files are how developers look at profiles: There was a problem hiding this comment. Right, now we're making progress! So (correct me if I'm getting you wrong), you're basically using "environment config" with the following meaning:
And I agree, this is a distinction we should make. I've given several reasons above to think that non-environment config is something we will want, whether sooner or later: I've given concrete suggestions for what those might be today, and I've also appealed to the fact that Temporal has a large product surface area, ambitions, and a lot of time in which it will exist in the future hence is likely to end up wanting this at some point. So unless we can identify reasons why we in fact do not think we'll ever want non-environment config, the challenge here is to design file-based configuration in a way that supports the following requirements (but not limited to these):
There was a problem hiding this comment.
But I think they should be rejected for config except for environment-specific suggestions, no matter how far in the future non-environment config is suggested. It doesn't mean other avenues for non-environment things can't be explored, I just don't think we need to account for them as part of this config which should be environment config only. I think we just have to decide if this config is environment-config only or not. I propose so, forever into the future. There was a problem hiding this comment.
Right, I just don't think we can use the name
Alternatively, perhaps we should be less strict and call it There was a problem hiding this comment. To be clear: I don't really like the idea of multiple files. I think users will think of it all as "config", and will be annoyed that Temporal has "multiple different config files", and so it will make the product seem more complicated. So, even though we might like to avoid a difficult design process at this stage, that might not be the right decision for the product. There was a problem hiding this comment. We can change the default filename, no problem. I have no strong preference on which alternative there is chosen. I suspect people won't really use it directly anyways very much because if they want to hand-edit a file they might put it somewhere more reasonable and provide that filename when loading or as an env var. Or they'll use the CLI. Similar to AWS config. I would be concerned however if we need to change the env var names or property names or the CLI commands or anything. I do think from a user POV we still need to treat this as "the config" and force qualifying "client" in those places. There was a problem hiding this comment.
This is the Goals section, but that should go in the Proposal section. The Goal should say what functional requirement we want to achieve: i.e. what experience/use cases we are aiming to enable for the user (or what bad experience we are aiming to avoid). For example "An SDK or CLI-based project can select one of multiple profiles". There was a problem hiding this comment. The goal is "single file configuration", I just worded it differently here |
||||||||||||||||||||||||||||||||||||||||||||||||
| * 💭 Why? | ||||||||||||||||||||||||||||||||||||||||||||||||
| * It is common in configurations like this to want to be able to share a single file with many configurations. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### Future Goals | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| None identified at this time. The goals are simple enough for the MVP and the non-goals are ones we never want. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Since the file is going to be named
to the "Future Goals" section. It's important that this be future-proof: we don't want a future where a second Temporal config file needs to be introduced. There was a problem hiding this comment. I can add that, sure. Granted I can't think of any non-client configuration at this time, but that's fine. |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### Non-goals | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| * Common format for declarative workers, schedules, etc. This is only for client options. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Let's go slowly here. In the future, if we want to support declarative configuration of other aspects of Temporal, then we will not want multiple config files. The natural design decision at this point is to say
In which case client config should be under its own top-level key, allowing other top-level keys in the future for all the things we're deliberately not designing or thinking about today: schedules, etc As long as we place the client config behind a top-level But if we were to make the config file client-specific, then these would need to be There was a problem hiding this comment. I think declarative resources should be tackled separately. Having said that, we can probably add non-client things in the future. But I'm not sure we need to qualify client things. The reason this is CLI/SDK configuration and not client configuration is because you need these things to basically do anything with the CLI/SDK. From a user POV it's just "config". If we want to add things that do not apply to clients (e.g. worker configuration) that may make sense, but I still think these general options can be top level. I think asking all users to add this There was a problem hiding this comment.
The proposed file name is
In the proposed schema, all the keys related to client config are already nested below But 6 is just the number in the first iteration of the proposal; it's bound to grow over time, and the result will be that the other sections we add to the file will be their own root nodes, swimming oddly among a sea of client config details. If we look at file formats such as TOML and TOML-ish things like I can only see advantage and no disadvantage in grouping the client config under its own key (e.g. There was a problem hiding this comment.
But there are no other sections today. I don't agree we should harm developer experience today for some may-never-happen unrealized future.
I mentioned this in the last paragraph above:
I don't want everyone to qualify these things as client-specific config just because there is technically the possibility of some non-client future. Looking at a previous comment, I think we have different ideas on the purposes of this configuration. This is environment/endpoint-and-auth-like configuration only. It is not code configuration, it is not meant to be committed, it is only for things that may change from one environment to the next. There are dozens of "options" that Temporal accepts for all sorts of things, but these are not what is meant by this environment configuration. I can try to clarify this in the document. |
||||||||||||||||||||||||||||||||||||||||||||||||
| * File-based hierarchy. We don't need many levels of file merging for these options. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Use external configuration by default in SDKs (it is default in CLI and samples). Users need to opt-in to this for | ||||||||||||||||||||||||||||||||||||||||||||||||
| compatibility and clarity reasons (but it is of course easy). | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### High-level Usage Examples | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. To add: a more obvious example of using a config file and configuration for vscode extension |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -70,7 +81,7 @@ you could do this in environment variables for mTLS auth: | |||||||||||||||||||||||||||||||||||||||||||||||
| export TEMPORAL_TLS_CLIENT_CERT_PATH=path/to/my/client.pem | ||||||||||||||||||||||||||||||||||||||||||||||||
| export TEMPORAL_TLS_CLIENT_KEY_PATH=path/to/my/client.key | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Now running the same code again unmodified will run against cloud. You can do this same thing with config instead of | ||||||||||||||||||||||||||||||||||||||||||||||||
| environment variables: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| temporal config set --key address --value my-ns.a1b2c.tmprl.cloud:7233 | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -97,16 +108,36 @@ Similar to AWS tooling and other tools, you can have profiles. So you can have: | |||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| For dev and then in production, this might exist: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| temporal config set --profile prod --key address --value my-prod-ns.a1b2c.tmprl.cloud:7233 | ||||||||||||||||||||||||||||||||||||||||||||||||
| temporal config set --profile prod --key namespace --value my-prod-ns.a1b2c | ||||||||||||||||||||||||||||||||||||||||||||||||
| temporal config set --profile prod --key tls.clientCertPath --value path/to/my/prod-cert.pem | ||||||||||||||||||||||||||||||||||||||||||||||||
| temporal config set --profile prod --key tls.clientKeyPath --value path/to/my/prod-cert.key | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Or alternatively it could be a fixed file that that creates or environment variables or whatever. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Now a simple setting of `TEMPORAL_PROFILE` environment to `dev` or `prod` will switch connectivity (or `--profile` on | ||||||||||||||||||||||||||||||||||||||||||||||||
| CLI or the string in the SDK if you don't want to use environment variable). | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| #### Manually Working with Configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| If a user wanted to hand-write a configuration, they can have `my-file.json` like: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ```json | ||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "profiles": { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "default": { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "address": "my-host:7233", | ||||||||||||||||||||||||||||||||||||||||||||||||
| "namespace": "my-namespace" | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| * This is the same format used by the CLI and the CLI can even be used to edit this file. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `TEMPORAL_CONFIG_FILE` env var can point to this file or the config file can be manually set in the CLI/SDK. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * There is a type safe contract this file conforms to for anyone wanting to work with it programmatically. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * All SDKs accept the type safe model this file represents if users would rather provide config that way. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| #### Possible UI Creation Flow | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| When a user creates an API key, the completion screen can provide a download of the config file that will have the | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -159,8 +190,8 @@ whether this is done by default can be discussed, but the idea is the same. | |||||||||||||||||||||||||||||||||||||||||||||||
| * `namespace` - Client namespace. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `apiKey` - Client API key. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `tls` - A boolean (true is same as empty object) _or_ an object. Note the default for this value is dependent | ||||||||||||||||||||||||||||||||||||||||||||||||
| upon other settings here. 💭 Why? We regret not making TLS the default in the past, so we will make it default | ||||||||||||||||||||||||||||||||||||||||||||||||
| if `apiKey` is present. If TLS is an object, it can have the following possible fields: | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `clientCertPath` - File path to mTLS client cert. Mutually exclusive with `clientCertData`. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `clientCertData` - Cert data. Mutually exclusive with `clientCertPath`. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * `clientKeyPath` - File path to mTLS client key. Mutually exclusive with `clientKeyData`. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -254,6 +285,26 @@ default is assumed. | |||||||||||||||||||||||||||||||||||||||||||||||
| values often contain commas. Would rather not force users to have an index in the env var name. Should we just not | ||||||||||||||||||||||||||||||||||||||||||||||||
| support gRPC meta from env var until we think this through? | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### Loading Configuration | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Configuration is loaded in the following manner: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| * Try to load profile from configuration file | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Only if file loading is enabled | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Can If user provides specific config file, use that, otherwise use the default location | ||||||||||||||||||||||||||||||||||||||||||||||||
| * If user specifies the profile, use that, otherwise use the default | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Try to overwrite with specific configuration environment variables | ||||||||||||||||||||||||||||||||||||||||||||||||
| * Only if env loading is enabled | ||||||||||||||||||||||||||||||||||||||||||||||||
| * If user specifies the profile, use that, otherwise use the default | ||||||||||||||||||||||||||||||||||||||||||||||||
| * If the `default` profile is being loaded, try the profile-specific environment variable first, e.g. | ||||||||||||||||||||||||||||||||||||||||||||||||
| `TEMPORAL_DEFAULT_ADDRESS` is used over `TEMPORAL_ADDRESS` if it's present. | ||||||||||||||||||||||||||||||||||||||||||||||||
| * The `default` env vars are only for the default profile, meaning a profile of `foo` cannot use `TEMPORAL_ADDRESS`, | ||||||||||||||||||||||||||||||||||||||||||||||||
| only `TEMPORAL_FOO_ADDRESS` if present. | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Minor clarity cleanups:
Suggested change
There was a problem hiding this comment. LGTM, will incorporate on my next commit |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| That is it, nothing more complicated. When the configuration is provided to the SDKs/CLI, they apply their normal logic | ||||||||||||||||||||||||||||||||||||||||||||||||
| which includes defaults for things that can be defaulted, or errors if they expect something that is not provided (this | ||||||||||||||||||||||||||||||||||||||||||||||||
| is SDK specific). | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ## Implementation | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| ### CLI | ||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. As above, we also must deprecate CLI env variables that differ from the new scheme. There was a problem hiding this comment. Correct, this was discussed above (and if we happen to have any other clients that use their own made-up/inconsistent env vars, but luckily I think it's only CLI) |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to be idiomatic per language? e.g. Java probably has its own configuration solution that's different from Python, that's different from .NET, etc.
I know this would be sub-optimal from the standpoint of running samples, so I'm not actually suggesting we go this route, but I do want to make sure we've explicitly considered (and thus written down) the trade-offs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not IMO. Most runtimes actually don't have this that I'm aware of, including Java, Python, .NET, etc. Also, we want the same config to work everywhere. If you're aware of somewhere where this is not idiomatic, can discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these goals seem more must-have than others. Can we categorize them into P0/P1/P2, and then maybe P3=Non goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these can be separate as they apply to the system as a whole. I don't think there's a clear split point with these goals that can be done in the future. The MVP is meant to be a very simple approach to accomplishing the goals, and everything else can be done in the future. The only place where I think work can be split is by language. Otherwise, these goals all amount to basically one feature. For this project, due to its simplicity in implementation, there were no "future goals" like there are in other projects. I will add a "Future Goals" section to clarify.
EDIT: Added "Future Goals" section, but it has nothing there right now. But we can add anything we think of (I can't think of anything here).