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

Add a migration guide #2199

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add a migration guide #2199

wants to merge 2 commits into from

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Feb 18, 2025

Motivation:

Moving from 1.x to 2.x isn't trivial. We should provide a guide for doing this.

Modifications:

  • Add a guide for migrating client code and services
  • Add a script which can automate parts of this

Result:

Some guidance on migrating.

Motivation:

Moving from 1.x to 2.x isn't trivial. We should provide a guide for
doing this.

Modifications:

- Add a guide for migrating client code and services
- Add a script which can automate parts of this

Result:

Some guidance on migrating.
@glbrntt glbrntt requested a review from gjcairo February 18, 2025 15:15
@glbrntt glbrntt added the semver/none No version bump required. label Feb 18, 2025
Copy link
Collaborator

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up. I have a few questions here:

This seems to only outline a migration path for the simplest scenarios. What if an application author has multiple dependencies that use gRPC v1

  • Do I have to wait until all of them vendored v1 so that I can depend on v2?
  • If multiple dependencies vendor gRPC v1 then we should probably advise them rename/alias the modules otherwise we will have conflicts

# The path of the checkout.
grpc_checkout_path="${grpc_checkout_dir}/grpc-swift-v1"
# Version of grpc-swift to checkout.
grpc_v1_tag="1.24.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be worth getting the latest tag by curling GH instead of hardcoding this here?

-exec sed -i '' 's/protoc-gen-grpc-swift/protoc-gen-grpc-swift-v1/g' {} +

# Update the path of the protoc plugin so it aligns with the target name.
log "Updating direcotry name for protoc-gen-grpc-swift-v1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log "Updating direcotry name for protoc-gen-grpc-swift-v1"
log "Updating directory name for protoc-gen-grpc-swift-v1"


1. Setup your package so it depends on a local copy of gRPC Swift 1.x and the
upstream version of 2.x.
2. Generate code for 2.x to alongside generated 1.x code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
2. Generate code for 2.x to alongside generated 1.x code.
2. Generate code for 2.x alongside generated 1.x code.

upstream version of 2.x.
2. Generate code for 2.x to alongside generated 1.x code.
3. Incrementally migrate targets to 2.x.
4. Remove the code generated for, and the dependency on 1.x.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
4. Remove the code generated for, and the dependency on 1.x.
4. Remove the code generated for 1.x, and the dependency on 1.x.

If you're reading this section then you're likely relying on metadata in your
service. This means you need to implement the `ServiceProtocol` instead of the
`SimpleServiceProtocol` and the transformations you need to apply are
aren't well suited automation. The best approach is to conform your
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
aren't well suited automation. The best approach is to conform your
aren't well suited for automation. The best approach is to conform your

Comment on lines +296 to +300
and return a `ServerResponse` or `StreamingServerResponse`. Request metadata is
available on the request object. For single responses you can set initial and
trailing metadata when you create the response. For streaming responses you can
set initial metadata in the initializer and return trailing metadata from the
closure you provide to the initializer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we link to the docs or tutorials in case this compressed version isn't enough to understand what's going on?

Comment on lines +395 to +398
If a generated client is passed into the function then duplicate the function
and replace the body of the new version with a `fatalError()`. You can also mark
it as deprecated to help you find usages of the function. You'll now have two
versions of the same function.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels like this paragraph and the one above beginning with "If the function is passed a generated client then duplicate it" should be merged.

is very similar, so you may not have to change any code. An important change to
highlight is that for RPCs which stream their responses you must handle the
response stream _within_ the closure passed to the client. By way of example,
imagine the following server streaming RPC from 1.x:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might also be helpful to link to appropriate tutorials/examples/docs from here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants