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

feat: support API versioning in generated protos #121

Merged
merged 3 commits into from
Apr 8, 2024

Conversation

vchudnov-g
Copy link
Collaborator

If the methods that wind up in a each proto service all have the same "apiVersion" field in the Discovery document, that value becomes part of a new "api_version" annotation in the proto service. If the version numbers for the methods in a service don't match, generation fails.

If the methods that wind up in a each proto service all have the same
"apiVersion" field in the Discovery document, that value becomes part
of a new "api_version" annotation in the proto service. If the version
numbers for the methods in a service don't match, generation fails.
// This method returns the same as parseMethods(), but returns all methods in a map indexed by
// method name.
//
// TODO: Combine with parseMethods?
Copy link

Choose a reason for hiding this comment

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

Suggested change
// TODO: Combine with parseMethods?
// TODO: See <REPLACE ME with link to new bug filed in https://github.com/googleapis/disco-to-proto3-converter/issues>

Same applies to the 2nd TODO comment in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,761 @@
// Copyright 2020 Google LLC
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2020 Google LLC
// Copyright 2024 Google LLC

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The date is derived from the Discovery revision field, so this is WAI. But to your point, since these Discovery fragments are new, I'll change them to have a 2024 date.

}

@Test
public void convertVersionedInconsistent() throws IOException {
Copy link

Choose a reason for hiding this comment

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

nit: Add a comment under each test which explains the purpose of each test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@parthea parthea assigned vchudnov-g and unassigned parthea Apr 7, 2024
@jskeet
Copy link

jskeet commented Apr 8, 2024

I don't know enough about this generator to give detailed feedback, but it looks fine to me.

@vchudnov-g vchudnov-g merged commit 7ce8f84 into main Apr 8, 2024
5 checks passed
@vchudnov-g vchudnov-g deleted the diregapic-apiversion branch April 8, 2024 20:53
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.

None yet

3 participants