Don't generate mix-ins when there aren't HTTP rules

[jskeet]

From https://google.aip.dev/client-libraries/4234

To be generated into a client library, a mixin service must be declared under apis in a google.api.Service. Furthermore, only the RPCs with google.api.http bindings declared in the http configuration of the same google.api.Service can be generated (more details on these bindings in AIP-127). If a mixin service RPC does not have a google.api.http rule declared in the google.api.Service, it must not be generated.

I believe we generate a mixin property based only on apis.

[jskeet]

(It would be interesting to see how many APIs this affects.)

I'll have a look at this next week to see how hard it is and what the impact is.

[jskeet]

(We can ignore the part about only generating some RPCs though, given the way we handle mixins.)

[jskeet]

Assuming my small test was correct, affects all of:

• Google.Cloud.AlloyDb.V1
• Google.Cloud.AlloyDb.V1Alpha
• Google.Cloud.AlloyDb.V1Beta
• Google.Cloud.BareMetalSolution.V2
• Google.Cloud.CapacityPlanner.V1Beta
• Google.Cloud.CloudBuild.V2
• Google.Cloud.ContactCenterInsights.V1
• Google.Cloud.DataCatalog.V1
• Google.Cloud.Datastream.V1
• Google.Cloud.DiscoveryEngine.V1
• Google.Cloud.DiscoveryEngine.V1Beta
• Google.Cloud.Dlp.V2
• Google.Cloud.Firestore.Admin.V1
• Google.Cloud.Firestore.V1
• Google.Cloud.Functions.V1
• Google.Cloud.Iam.V3
• Google.Cloud.Iam.V3Beta
• Google.Cloud.Retail.V2
• Google.Cloud.Run.V2
• Google.Cloud.Translate.V3
• Google.Cloud.VisionAI.V1
• Google.Cloud.Workstations.V1

I think we should check whether they're actually non-functional before removing anything. It feels odd to require an HTTP rule which may be exactly the same as the original rule.

[jskeet]

Okay, I've tested this now (using gRPC, and assuming a status code of Unimplemented means something is broken, whereas any other status code is okay), and it's a mixed bag.

• Google.Cloud.AlloyDb.V1
  • Would remove IAM, looks like it's functional
• Google.Cloud.AlloyDb.V1Alpha
  • Would remove IAM, looks like it's functional
• Google.Cloud.AlloyDb.V1Beta
  • Would remove IAM, looks like it's functional
• Google.Cloud.BareMetalSolution.V2
  • Would remove IAM; already broken. (Location still works and would be left.)
• Google.Cloud.CloudBuild.V2
  • Would remove locations; already broken. (IAM still works and would be left.)
• Google.Cloud.ContactCenterInsights.V1
  • Would remove IAM, looks like it's functional
• Google.Cloud.DataCatalog.V1
  • Would remove IAM, already broken
• Google.Cloud.Datastream.V1
  • Would remove IAM, looks like it's functional.
• Google.Cloud.DiscoveryEngine.V1
  • Would remove locations, already broken
• Google.Cloud.DiscoveryEngine.V1Beta
  • Would remove locations, already broken
• Google.Cloud.Dlp.V2
  • Would remove locations, already broken
• Google.Cloud.Firestore.Admin.V1
  • Would remove locations, looks like it's functional
• Google.Cloud.Firestore.V1
  • Would remove locations, looks like it's functional
• Google.Cloud.Functions.V1
  • Would remove IAM, looks like it's functional
• Google.Cloud.Iam.V3
  • Would remove IAM (v1), looks like it's functional
• Google.Cloud.Iam.V3Beta
  • Would remove IAM (v1), looks like it's functional
• Google.Cloud.Retail.V2
  • Would remove locations, looks like it's functional (although gives permission denied, oddly)
• Google.Cloud.Run.V2
  • Would remove locations, looks like it's functional
• Google.Cloud.Translate.V3
  • Would remove locations, already broken
• Google.Cloud.VisionAI.V1
  • Not released, so not tested (and don't need to worry about breaking users)
• Google.Cloud.Workstations.V1
  • Work remove locations, looks like it's functional

Proposal:

• Allow-list (hardcoded in the generator, at least for now) the clients that we would remove, but which appear to work
• Stop generating the rest (including for new APIs), and accept that this is a theoretically breaking change that shouldn't hurt anyone
• Check how we're detecting mixins in google-cloud-dotnet tooling (to avoid unnecessary dependencies)

Based on the above, the allow-list would be:

• Google.Cloud.AlloyDb.V1, IAM
• Google.Cloud.AlloyDb.V1Alpha, IAM
• Google.Cloud.AlloyDb.V1Beta, IAM
• Google.Cloud.ContactCenterInsights.V1, IAM
• Google.Cloud.Datastream.V1, IAM
• Google.Cloud.Firestore.Admin.V1, Locations
• Google.Cloud.Firestore.V1, Locations
• Google.Cloud.Functions.V1, IAM
• Google.Cloud.Iam.V3, IAM
• Google.Cloud.Iam.V3Beta, IAM
• Google.Cloud.Retail.V2, Locations
• Google.Cloud.Run.V2, Locations
• Google.Cloud.Workstations.V1, Locations

@amanda-tarafa: Does that sound like a reasonable plan to you?

[amanda-tarafa]

Yes, I think that's fine, with two notes:

• I'd remove the betas and alphas from the allow list, effectively breaking them now.
• I'd add a note to remove the allow list all together for our next major version, assuming guidance does not change.

What do you think?

[jskeet]

First bullet definitely makes sense.

For the second, I think we should probably talk with others - if this is working functionality that could be useful, I wonder whether it would be better to try to get the HTTP rules added in, and get it supported by all languages? Probably more internal discussion required.

[amanda-tarafa]

For the second, I think we should probably talk with others - if this is working functionality that could be useful, I wonder whether it would be better to try to get the HTTP rules added in, and get it supported by all languages? Probably more internal discussion required.

+ 1 to trying to get the rules added first where it makes sense, and ideally we don't need the allowlist after. My point is more about, we shouldn't keep such an allow list around indefinetely.