-
Notifications
You must be signed in to change notification settings - Fork 148
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
Experimental cloud operations client #2146
Conversation
setTarget(DEFAULT_CLOUD_TARGET); | ||
setEnableHttps(true); |
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 tried a few different ways of doing defaults for these, but there are struggles in each other way (e.g. making enable HTTPs a Boolean
to determine if unset, etc). Doing it here and forcing the newBuilder
overload to only accept a full CloudServiceStubsOption
to copy from was the best way I could devise with limited changes.
temporal-serviceclient/src/main/java/io/temporal/serviceclient/CloudServiceStubsOptions.java
Show resolved
Hide resolved
@Override | ||
public Builder setChannel(ManagedChannel channel) { | ||
// Unset our defaults | ||
setEnableHttps(false); |
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.
Why do we need this? setChannel
should be mutually exclusive with setEnableHttps
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.
There is this check in ServiceStubsOptions.Builder.validateAndBuildWithDefaults
:
if (this.enableHttps && this.channel != null) {
throw new IllegalStateException(
"Only one of the 'enableHttps' or 'channel' options can be set at a time");
}
So if someone wants to set a channel, they don't know they have to go unset enableHttps
that we defaulted to true. Earlier incarnations didn't do defaulting enableHttps
to true
eagerly this way, but they were all ugly.
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.
Probably not a big deal, but in principle I don't like overriding options user may have set. Why do you need to set the target and https in the builder constructor? couldn't you set it in validateAndBuildWithDefaults
if a channel is not set? Or just set them for DEFAULT_INSTANCE
?
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.
couldn't you set it in
validateAndBuildWithDefaults
if a channel is not set?
This is what I had originally but I struggled with the logic. How can you tell whether someone set enableHttps
to know if it should be overridden, it's a primitive boolean. If you change it to Boolean
in the builder to maintain a null/unset state that doesn't help if you are using the primitive in the actual options because most people use build
and then we internally use the newBuilder
override that accepts options to copy then validate and set defaults. Or do you want Boolean
even in the options and just change the getEnableHttps
to return enableHttps != null && enableHttps
? I can do that if we want.
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.
Wouldn't you leave enableHttps
false in the constructor so it is only true if a user enabled 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.
Only API key at this time (and maybe that will always be the case, unsure). Yes, we could technically have overloads that accept params and use the builder for you (same for workflow service too where we know common things people need).
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.
My vote is newCloudServiceStubs
has a required parameter for the API
key and sets all the parameters and that would be the encouraged way to create the cloud service stub.
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.
We are not requiring API key in any cloud ops client in any SDK. That the cloud API currently requires an API key in its experimental form doesn't mean it always will. We are trying to have users set API keys the same way for clients since they are expected to be the same API keys. So if you use a certain way to create a workflow client in a language, you can apply that knowledge to creating a cloud client. This is just a case of one client needing a different default for two fields regardless of how they come and configure auth later (which may override defaults).
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.
We are not requiring API key in any cloud ops client in any SDK
I just think it should be required for newCloudServiceStubs
not the CloudServiceStubIOptions
. I would just like the expected case, connecting to our cloud with an API key, to be as easy as possible. If the requirements change we can change the SDK API to reflect that. I don't think it is a big deal, hence I approved.
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 would just like the expected case, connecting to our cloud with an API key, to be as easy as possible.
This is going to start being the expected case with workflow service I think too (though there is self-hosted). I think we should consider making API keys as easy to use for everyone at the same time (even if it's used more often in one place than another).
I do think in the future we may want a simple "connect to cloud" that accepts API key, knows the target host (fixed target host is coming for API key), auto-enables HTTPs, etc. And I think we're going to want that to look the same for workflow service and cloud service.
What was changed
io.temporal.serviceclient.CloudServiceStubs
and impl and optionsio.temporal.client.CloudOperationsClient
as (currently) just a simple client that returns the raw service for useChecklist