[Kms] Regenerate with the new gapic config #1165
Conversation
googlebot
added
the
cla: yes
tmatsuo
added
the
api: cloudkms
There was a problem hiding this comment.
Nice! It's great to see the new generated proto updates coming through.
Since it is unlikely we have too many users on this at the moment and we are in beta, what do you think about ripping the bandaid off and removing the classes which use the old namespace structure? Is there a flag to toggle whether or not they get generated again in the future in the case we were to remove them now?
|
I'm ok deleting them! |
|
and removed! @dwsupplee PTAL |
|
Interesting. The unit test still refers to the old namespace and now it fails. On the second thoughts, keeping the old namespace doesn't harm anything, and according to the comment, the future version will remove the old namespaces. What do you think about keeping them? |
|
Good eye on that. After looking a bit more, the code samples in the GAPIC reference the old namespaces as well. This ends up teaching users to utilize the deprecated approach which will eventually break them. Since we have the tools available to us to avoid that to a large degree right now, I still lean towards doing what we can to remove the deprecated classes. What do you think about us taking the time to update the GAPIC tooling to remove the usage of the old namespaces before releasing this? |
|
Yeah that seems like the right thing to do. We need to remove the old namespace usage in the code sample in the client at least. The old namespace is also used in Filed a bug: googleapis/gapic-generator#2141 |
Add back the deprecated files
|
Until we have a fix in gapic-generator, maybe we can have a patch in our synthtool script for the code sample. Other usages are not user visible, so not a big problem. @dwsupplee PTAL |
|
That sounds like a good compromise until we have this addressed in the GAPIC generation. I do have one slight concern, which I am on the fence about. Please let me know what you think. The deprecated classes trigger a warning about being removed in the next major release. It seems as though this warning is intended for the next major release of protobuf - but since we are publishing the classes in this library it strikes me as though we should be following our own guidelines instead. My hope would be that since we are in beta we can remove the deprecated classes sooner than when we flip KMS to GA. Do you think it would also be worth it to use the synth tool to update the warning messages? |
|
@dwsupplee Very good point. Thanks for pointing out. I updated the wording for the deprecation warning. PTAL! |
* Add isRacy() to SafeSearch (#1166) The SafeSearch class included `is` functions for each SafeSearch feature (adult, spoof, medical, violence), but was missing Racy. This Pull Request adds `isRacy()` to the SafeSearch class. * Exclude component vendor folder from snippet coverage (#1168) cc @tmatsuo * Allow context to handle Throwable interface (#1164) * Add getServiceAccount to the BigQueryClient (#1167) * Add getServiceAccount to the BigQueryClient See: https://cloud.google.com/bigquery/docs/reference/rest/v2/projects/getServiceAccount * Added $options to getServiceAccount and fixed the indent * Use single quote * Add snippet test * Comment update, use self instead of $this * Comment update * Prepare v0.71.0 (#1169) * Prepare v0.71.0 * patch release for Logging * Correct version for Logging * [Kms] Regenerate with the new gapic config (#1165) * Update for the new gapic configuration * Docs update for the new and nicer namespace * Removed the files with the old namespace * Use the new namespace in the code sample Add back the deprecated files * Changed the wording for the deprecation warning * Fix firestore queries (#1161) * Bump gax to 0.35 (#1170) * Add an interactive release builder. (#1160) * Add an interactive release builder. * Create build directory if it doesn't exist * Add getServiceAccount method to StorageClient (#1173) * Re-generate library using Tasks/synth.py (#1174) * Re-generate library using Tasks/synth.py * Use new namespace in user visible area, tweak the deprecation wording * Add support for Numeric type (#1172) * Add support for Numeric type * Added a cast in Numeric's constructor, added system tests * Allow '123.' and '.123', update tests * [Breaking Change] Add support for Document Snapshots in Firestore Query Cursors (#1162) cc @schmidt-sebastian Extracted from #923 and updated to address pull request comments. Breaking change is the standardization in Query of using `InvalidArgumentException`, replacing various throws of `BadMethodCallException`. Closes #851. * Fix Storage Requesterpays system tests (#1180) The requester pays system tests have been broken for some time. This change fixes them. * Bandaid for protobuf 4761 (#1176) * Temporary workaround for protobuf extension issue protocolbuffers/protobuf#4761 * Bump gax to 0.36 * Configure comparators for the unit test * Revert back to normal TestCase * Install protobuf extension in the PHP 7.2 test runner * Revert to TestCase * Prepare v0.72.0 (#1181) * Added a document for the time filter on BigQueryClient->jobs() (#1183) * Narrowed the time filter in the system test (#1185) * Re-generate library using BigQueryDataTransfer/synth.py (#1184) * Re-generate library using BigQueryDataTransfer/synth.py * Tweak the wording on the deprecation warning Also use the new namespace in the sample code * pin auth version until we have a way to silence warnings (#1189)
No description provided.