-
Notifications
You must be signed in to change notification settings - Fork 174
Add support for environment config #2660
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
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,13 @@ | |||
description = '''Temporal Java SDK Environment Config Module''' |
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.
In terms of naming it would probably more consistent internally in the Java SDK to call this module temporal-enviorment-configuration
, but that isn't consistent with the Go SDK
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.
👍 The envconfig
shortened term for the modules/packages/namespaces myself is kinda nice
} | ||
} | ||
|
||
public ClientConfig(Map<String, ClientConfigProfile> profiles) { |
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.
May want to use a builder here for consistency
|
||
/** ClientConfig represents a client config file. */ | ||
@Experimental | ||
public class ClientConfig { |
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.
Hrmm, ideally we could have one static call on client config in the SDKs that gives you what you need to connect a client. With Java this is admittedly harder because 1) stubs are separate from client, and 2) Java doesn't support a native tuple to return two things at once.
I wonder if there is any creative approach we can think of here.
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.
The only thing I could think of was have a method that gives you a full client that takes a few options customizers to set additional options on the service stub options and client options.
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 think what you have here is good enough
|
||
/** ClientConfig represents a client config file. */ | ||
@Experimental | ||
public class ClientConfig { |
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 know it sounds pedantic, but can we also expose static calls for getting the default profile stuff to connect. Specifically, can we have:
public static WorkflowServiceStubsOptions loadWorkflowServiceStubsOptions()
and
public static WorkflowClientOptions loadWorkflowClientOptions()
on this class? Or maybe named loadDefaultWorkflowServiceStubsOptions
or something? With the other SDKs, we are putting a static call on the highest level class/place we can to be able to load what is needed from the default profile to make client connections. I would guess these would be the calls used by every SDK sample.
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.
So we wan't a short cut to call ClientConfigProfile.load().toWorkflowServiceStubsOptions()
?
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.
Yeah, we have been adding this to other SDKs. But it's not a big deal if we don't do it.
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.
One feature we were adding to other SDKs is the ability to be able to read/write TOML. For languages that didn't want a dependency, we would just give an object that could serialize as such, but Java already has the dependency.
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.
So the Go SDK does not have the feature, does any SDK support the ability to read/write the TOML file? Is any user asking for this?
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.
So the Go SDK does not have the featur
ClientConfig
does have FromTOML
and ToTOML
does any SDK support the ability to read/write the TOML file? Is any user asking for this?
Every SDK supports at least the raw TOML structure (even if it's maps/dictionary structures of generic objects). We don't necessarily do the reading/writing of TOML to an actual file for the users, but the ability for users to parse/write our config format in SDKs was something we had originally stated was a goal. Doesn't have to be TOML or even typesafe structures, just any way a user can parse/write it.
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.
Okay so we want Okay I added a fromToml
and toToml
equivalent
return new Builder(config); | ||
} | ||
|
||
private final boolean disabled; |
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.
IMO we should match the TOML representation exactly, which IMO I think means this is a Boolean
because it can be unset and unset has different behavior than just false (default is TLS disabled by default unless API key is present). This is mostly thinking about the ability to convert it back to TOML, but maybe that's not a concern.
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.
does this also apply to disableHostVerification
as well?
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.
Hrmm, up to you since it's only ever true/false (absent is always false). My concern was mostly about disabled's absence having a special meaning, granted in Go we don't use a nullable bool either, hrmm. No strong opinion here. Core-based SDKs do not have a disable-host-verification option.
@@ -0,0 +1,13 @@ | |||
description = '''Temporal Java SDK Environment Config Module''' |
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.
👍 The envconfig
shortened term for the modules/packages/namespaces myself is kinda nice
c130d9f
to
471a1ae
Compare
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.
LGTM (especially since it's experimental atm). Added small suggestion concerning Jackson version.
471a1ae
to
9d28fe9
Compare
Add support for environment config to the Java SDK
https://docs.temporal.io/develop/environment-configuration