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.

Sign up for GitHub

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

Jump to bottom

Add Cloud API #157

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Add Cloud API #157

merged 1 commit into from
Apr 23, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented Apr 15, 2024

What changed?

Part of temporalio/features#440. Notes:

  • Added cloud API submodule from https://github.com/temporalio/api-cloud
  • I despise Makefiles, but went with what has already been done here
  • Enum fixing does not apply to cloud API because it doesn't use enums
  • Auto-update is only for the API repo (i.e. automatically update this repository on merge of that one) since it has the same proto checks as this repo and therefore updates cleanly, but cloud API submodule must be updated manually (cloud API not in API repo at this time)
  • OpenAPI spec for cloudAPI is not present because that is from the API repo (cloud API not in API repo at this time)
  • Cloud API cannot share any API repo models (cloud API not in API repo at this time)
  • Expecting in the future cloud API will be in API repo and the cloud-specific aspects will go away

@cretz cretz requested review from mastermanu and anekkanti April 15, 2024 16:12
@cretz cretz requested review from a team as code owners April 15, 2024 16:12
bergundy
bergundy reviewed Apr 15, 2024
View reviewed changes
Makefile
Comment on lines +144 to +145
# Delete entire cloud dir
rm -rf cloud
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the line above delete this dir?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came from my last tests with #155 where I believe it only worked one directory deep, but I can test again.

@cretz cretz requested a review from a team April 15, 2024 18:54
@cretz cretz mentioned this pull request Apr 15, 2024
Open
8 tasks
@alexshtin
Copy link
Member

Enum fixing does not apply to cloud API because it doesn't use enums

I am sure, Cloud API will have enums one day.

alexshtin
alexshtin approved these changes Apr 17, 2024
View reviewed changes
README.md
```

## Rebuild

Run `make` once to install all plugins and tools (`protoc` and `go` must be installed manually).

Run `make update-proto` to update submodule and recompile proto files.
Run `make update-proto` to update the `proto/api` submodule and recompile proto files. The `proto/api-cloud` submodule
must be updated manually.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"manually" because you didn't create a make target for it or is there another reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I didn't want this to be with that other make target because that is run automatically via GH workflow, and we don't want to pull in this repo automatically because it may break in here (there are not checks in that repo that match this repo like the api repo has). So updating this submodule is manual, as opposed to a make target auto-run by GH workflow.

Makefile
Comment on lines +83 to +85
mockgen -package operatorservicemock -source operatorservice/v1/service_grpc.pb.go -destination operatorservicemock/v1/service_grpc.pb.mock.go
mockgen -package workflowservicemock -source workflowservice/v1/service_grpc.pb.go -destination workflowservicemock/v1/service_grpc.pb.mock.go
mockgen -package cloudservicemock -source cloud/cloudservice/v1/service_grpc.pb.go -destination cloud/cloudservicemock/v1/service_grpc.pb.mock.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

Copy link
Member Author

@cretz cretz Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? (it was silly to have all these lines of complexity for two lines of mockgen call)

@cretz
Copy link
Member Author

cretz commented Apr 17, 2024

I am sure, Cloud API will have enums one day.

They have decided that they never want them (I disagree with their decision)

@cretz cretz merged commit e50aa65 into temporalio:master Apr 23, 2024
3 checks passed
@cretz cretz deleted the cloud-api branch April 23, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anekkanti anekkanti anekkanti approved these changes

@alexshtin alexshtin alexshtin approved these changes

@bergundy bergundy bergundy approved these changes

@mastermanu mastermanu Awaiting requested review from mastermanu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cretz @alexshtin @bergundy @anekkanti