fix(nodejs): remove all common proto configuration#6277
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the omit_common_resources configuration option and its associated logic across the codebase, including documentation, configuration structures, generation logic, migration tools, and tests. Feedback on the changes suggests simplifying a redundant conditional check and removing a redundant Path field when instantiating the NodejsAPI configuration in the migration tool.
|
I've run "migrate and regenerate everything" with this PR, and:
|
|
For posterity, we discussed that removing common_protos will impact some generated code in certain packages, such as secretmanager; we must first examine the removed code to determine if its absence is acceptable before deciding whether to proceed with the change. Reproduction stepsmigrate .add default:
keep:
- package.json
- samples/package.json
- README.md
- CHANGELOG.md
- .readme-partials.yaml
- librarian.js. # <---- add thislibrarian generate google-cloud-secretmanagergit diff packages/google-cloud-secretmanager/src/v1/secret_manager_service_client.ts |
|
Marked as "do not merge" because this changes the generated code - we no longer generate code for project resources, for example. I'll look into it in more detail. |
The common resources proto is unconditionally passed into the gapic generator so that the generator knows about the common resource patterns and can generate helper functions, but is then unconditionally deleted from the protos directory. The only reason we currently have those files in google-cloud-node is because they were included at one point and never cleaned up. After removal, the code is simpler and migration won't end up creating unnecessary files. Trying to reproduce the Bazel behavior to match which proto files are included and which are copied would be very complicated; this approach is simpler (albeit less "clean" feeling). Fixes googleapis#6024
|
This is now changed to always pass in google/cloud/common_resources.proto to the generator, and always remove it from the protos directory afterwards. |
|
(I've regenerated everything, and the diffs we were seeing are no longer present.) |
|
This does create a diff for advisory-notifications (and potentially some other APIs), but I think it's okay - it basically adds path templates that aren't strictly necessary. Unfortunately advisory-notifications has a large set of other diffs, so it's tricky to see what's just due to this change, but I think we're okay. |
quirogas
left a comment
There was a problem hiding this comment.
I think we should still keep the ability to omit common protos for now to keep the diff small. Perhaps we could rename the flag to reflect that it doesn't have any resources and keep it?
|
Converting back to a draft for now while we're discussing it. |
The common resources proto is unconditionally passed into the gapic
generator so that the generator knows about the common resource
patterns and can generate helper functions, but is then
unconditionally deleted from the protos directory. The only reason we
currently have those files in google-cloud-node is because they were
included at one point and never cleaned up.
After removal, the code is simpler and migration won't end up creating
unnecessary files.
Trying to reproduce the Bazel behavior to match which proto files are
included and which are copied would be very complicated; this approach
is simpler (albeit less "clean" feeling).
Fixes #6024