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

Sync to latest API Cloud to include Export #190

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

alice-yin
Copy link
Contributor

What changed?

Why?

How did you test it?

Potential risks

@alice-yin alice-yin requested review from a team as code owners November 13, 2024 00:18
@alice-yin alice-yin force-pushed the aliceyin/sync_api_cloud_export_change branch from 2753319 to c47afb0 Compare November 13, 2024 00:21
@alice-yin alice-yin force-pushed the aliceyin/sync_api_cloud_export_change branch from c47afb0 to a4cd3c7 Compare November 13, 2024 00:24
@alice-yin alice-yin changed the title adding new api cloud change Sync to latest API Cloud to include Export Nov 13, 2024
@bergundy
Copy link
Member

Looks like a lot of unrelated files were changed in this PR. I would guess due to using a different version of the protoc plugin. Can you confirm that this is intentional?

@cretz
Copy link
Member

cretz commented Nov 13, 2024

And can you confirm this is a result of running make proto and committing everything instead of just cloud things? We really need a step in CI that checks generated code didn't change.

@alice-yin
Copy link
Contributor Author

@cretz @bergundy

I only added the cloud related files. It has a lot of files as we haven't synced changes in this repo for a while. But if I'm running make proto in the whole repo, a lot of other files got pulled in. Example pr: https://github.com/temporalio/api-go/pull/189/files

Do we want to do that as well? Is it from previous pr?

@cretz
Copy link
Member

cretz commented Nov 14, 2024

But if I'm running make proto in the whole repo, a lot of other files got pulled in

Looking at the diff on those, it looks like the copyright headers are removed and your version of protoc may be different than what was here before. I'd have to set aside time to investigate, but in theory if your running of the make command ends up with anything different than what's already in master for non-cloud things, something may be different with your environment or how it is run. master is automatically kept up to date with the API repo.

@alice-yin alice-yin force-pushed the aliceyin/sync_api_cloud_export_change branch from 9c23bba to 8d254f9 Compare November 15, 2024 00:27
@alice-yin
Copy link
Contributor Author

yeah. you're right. Running make helps me update a lot of dependencies. Previously I'm running make proto @bergundy @cretz this is up for review now.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will you also be doing this for the cloud git submodule inside https://github.com/temporalio/sdk-java/tree/master/temporal-serviceclient/src/main and the cloud git subtree inside https://github.com/temporalio/sdk-core/tree/master/sdk-core-protos/protos (see https://github.com/temporalio/sdk-core?tab=readme-ov-file#proto-files)?

We definitely don't want to favor Go as if it is more important than other languages.

@cretz cretz mentioned this pull request Nov 15, 2024
@alice-yin alice-yin enabled auto-merge (squash) November 18, 2024 22:06
@alice-yin alice-yin merged commit 04ae3de into master Nov 18, 2024
3 checks passed
@alice-yin alice-yin deleted the aliceyin/sync_api_cloud_export_change branch November 18, 2024 22:06
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.

3 participants