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

Experimental cloud operations client #2146

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Experimental cloud operations client #2146

merged 3 commits into from
Jul 19, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented Jul 17, 2024

What was changed

  • Added api-cloud protos as new submodule and included in proto gen
  • Added io.temporal.serviceclient.CloudServiceStubs and impl and options
  • Added io.temporal.client.CloudOperationsClient as (currently) just a simple client that returns the raw service for use
  • Added CI test

Checklist

  1. Closes Cloud Operations API Client #2059

Comment on lines +87 to +88
setTarget(DEFAULT_CLOUD_TARGET);
setEnableHttps(true);
Copy link
Member Author

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.

.github/workflows/ci.yml Show resolved Hide resolved
@cretz cretz marked this pull request as ready for review July 17, 2024 17:14
@cretz cretz requested a review from a team as a code owner July 17, 2024 17:14
@Override
public Builder setChannel(ManagedChannel channel) {
// Unset our defaults
setEnableHttps(false);
Copy link
Contributor

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

Copy link
Member Author

@cretz cretz Jul 17, 2024

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns Jul 17, 2024

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?

Copy link
Member Author

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).

Copy link
Contributor

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.

Copy link
Member Author

@cretz cretz Jul 18, 2024

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).

Copy link
Contributor

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.

Copy link
Member Author

@cretz cretz Jul 19, 2024

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.

@cretz cretz merged commit 0ba6188 into master Jul 19, 2024
11 checks passed
@cretz cretz deleted the cloud-ops-api branch July 19, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloud Operations API Client
2 participants