Skip to content

Conversation

@noahdietz
Copy link
Collaborator

@noahdietz noahdietz commented Oct 29, 2025

Pulls out the google.api.api_version service-level annotation and includes it in the gapic_metadata.json services entry.

Also refactors gapic_metadata generation to be a little more picky about when things are called because we don't want to silently initialize things and potentially miss adding information we need i.e. the service-level ApiVersion annotation. This shouldn't be a big deal though because these are always called in a hierarchical way, as the service and then methods are traversed and once for each transport.

Internal bug http://b/452365074

@noahdietz noahdietz marked this pull request as ready for review October 29, 2025 22:25
@noahdietz noahdietz requested a review from a team as a code owner October 29, 2025 22:25
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

The early return behavior feels a bit itchy to me, but I realize that's an inherited behavior from the proto API.

@noahdietz
Copy link
Collaborator Author

The early return behavior feels a bit itchy to me, but I realize that's an inherited behavior from the proto API.

Yeah...it's a bit weird...my feeling was that if something was called out of order we either 1. return an error (not doing that yet today, so seemed a bit overkill) or 2. do nothing, emit nothing

The latter seemed preferred as it would be obvious in tests and generated output that something is missing.

I'm happy to change it back to the "silently inits its parent" behavior. It would just be "lossy" if called out of order b.c not every scope has access to apiVersion information.

@shollyman
Copy link
Contributor

The early return behavior feels a bit itchy to me, but I realize that's an inherited behavior from the proto API.

Yeah...it's a bit weird...my feeling was that if something was called out of order we either 1. return an error (not doing that yet today, so seemed a bit overkill) or 2. do nothing, emit nothing

The latter seemed preferred as it would be obvious in tests and generated output that something is missing.

I'm happy to change it back to the "silently inits its parent" behavior. It would just be "lossy" if called out of order b.c not every scope has access to apiVersion information.

I've got some intentions wrt init and setup of this plugin so that we validate things a bit more strongly on startup. I'll keep this in mind when I revisit that work.

@shollyman shollyman changed the title feat(internal/gengapic): include API verison in gapic metadata feat(internal/gengapic): include API version in gapic metadata Oct 30, 2025
@noahdietz noahdietz merged commit a098fba into googleapis:main Nov 3, 2025
7 checks passed
@noahdietz noahdietz deleted the gapic-metadata-api-version branch November 3, 2025 17:47
Copy link

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Late I know.

if md, ok := g.metadata.GetServices()[tst.serv.GetName()]; !ok {
t.Errorf("ClientInit(%s) gapic metadata, expected %s to be present but found %+v", tst.tstName, tst.serv.GetName(), g.metadata.GetServices())
} else if tst.serv == servPlain && md.GetApiVersion() != servPlainAPIVersion {
t.Errorf("ClinitInit(%s) gapic metadata service entry api version, got %q, want %q", tst.tstName, md.GetApiVersion(), servPlainAPIVersion)
Copy link

Choose a reason for hiding this comment

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

nit: typo, ClinitInit -> ClientInit

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