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

Update protos #647

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Update protos #647

merged 3 commits into from
Dec 4, 2023

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison commented Nov 26, 2023 •
edited
Loading

Update protos (needed for Typescript Update work).

This PR sets the new sdk_name and sdk_version fields to their zero values. See #588

This was referenced Nov 26, 2023
Open
Merged
@dandavison
Copy link
Contributor Author

Linter violations are fixed in #651

@dandavison dandavison marked this pull request as ready for review December 4, 2023 12:22
@dandavison dandavison requested a review from a team as a code owner December 4, 2023 12:22
cretz
cretz reviewed Dec 4, 2023
View reviewed changes
core/src/internal_flags.rs
Comment on lines +143 to +144
sdk_name: "".to_string(),
sdk_version: "".to_string(),
Copy link
Member

@cretz cretz Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we want to set these to client_name and client_version from the client if they are different from the previous task (or remain empty/unset if they are the same).

The PR description says "See temporalio/features#321" but this doesn't solve #588 completely. Is this just a temporary thing until #588 can be properly addressed?

Copy link
Contributor Author

@dandavison dandavison Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's right. There's a little bit more work and testing required for that because we need to be watching for changes from task-to-task, rather than sending the same value repeatedly. So that's #588. This PR is just for unblocking Typescript work so I was thinking that it's fine to merge this as-is since these changes will result in transmitting the protobuf default zero-values, which is what the server is already using.

@dandavison dandavison merged commit b1e5e4b into temporalio:master Dec 4, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cretz cretz cretz approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@dandavison @cretz