feat(generator): generate a SetIamPolicy() overload with an OCC loop #7276
Conversation
For generated clients with standard `GetIamPolicy()` and `SetIamPolicy()` operations, also generate a `SetIamPolicy()` overload that implements an optimistic concurrency control loop, where the `Policy.etag` field is used to prevent simultaneous updates of a policy from overwriting each other. Fixes googleapis#7168.
google-cla
bot
added
the
cla: yes
googlebot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7276 +/- ##
========================================
Coverage 94.34% 94.34%
========================================
Files 1317 1317
Lines 114594 114799 +205
========================================
+ Hits 108109 108304 +195
- Misses 6485 6495 +10
Continue to review full report at Codecov.
|
| " * Updates the IAM policy for @p resource using an" | ||
| " optimistic concurrency control loop.\n" | ||
| " *\n" | ||
| " * The loop fetches the current policy for @p resource, and" |
There was a problem hiding this comment.
nit: the lack of \n in these comments make the output into a very long line. Seems like you decided to rely on clang-format? As an alternative, consider using a raw string to make the formatting more obvious?
There was a problem hiding this comment.
Done, although in the long run I suspect we may well be better off relying on clang-format to do all the formatting rather than only part of it.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @devbww)
generator/internal/client_generator.cc, line 76 at r2 (raw file):
"google/cloud/polling_policy.h", "google/cloud/status_or.h", "google/cloud/version.h"}); if (get_iam_policy_extension_ && set_iam_policy_extension_) {
There's no guarantee (that I'm aware of) that either or both GetIamPolicy and SetIamPolicy rpcs have method_signature options. It's just proved true so far in the handful of services we've encountered. It might be better to use the proto request/response typed rpcs directly.
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @scotthart)
generator/internal/client_generator.cc, line 76 at r2 (raw file):
Previously, scotthart (Scott Hart) wrote…
There's no guarantee (that I'm aware of) that either or both GetIamPolicy and SetIamPolicy rpcs have method_signature options. It's just proved true so far in the handful of services we've encountered. It might be better to use the proto request/response typed rpcs directly.
The logic was that if they don't have the "resource" and "resource,policy" signature extensions respectively, then we don't want to generate a "resource,updater" extension either. That is, if the service only uses the proto request/response types, then that is all you get.
There was a problem hiding this comment.
Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @devbww)
| @@ -26,6 +26,7 @@ google_cloud_cpp_common_hdrs = [ | |||
| "iam_binding.h", | |||
| "iam_bindings.h", | |||
| "iam_policy.h", | |||
| "iam_updater.h", | |||
There was a problem hiding this comment.
This seems like the wrong place for these headers, since they seem to depend on gRPC and this list is for the "common" deps, which shouldn't have a dep on grpc. I suspect the other iam-related headers that already existed here may be in the wrong place too.
There was a problem hiding this comment.
iam_updater.h moved in #7287.
For generated clients with standard
GetIamPolicy()andSetIamPolicy()operations, also generate a
SetIamPolicy()overload that implements anoptimistic concurrency control loop, where the
Policy.etagfield isused to prevent simultaneous updates of a policy from overwriting each
other.
Fixes #7168.
This change is