fix: revert dict back to protobuf in the iam binding update #1838
Conversation
This is needed as some recent updates to a dependency (google-cloud-resource-manager or its dependencies) broke the existing logic, and we are seeing the
error like this:
--> policy.bindings.append(new_binding)
TypeError: Expected a message object, but got {'role': 'roles/run.invoker', 'members': [...]}
product-auto-label
bot
added
size: xs
|
Let's try to update google3 first. That google.iam namespace is indeed problematic |
|
CC @parthea Did the gapic generator make a breaking change to no longer allow dictionary inputs? |
There was a problem hiding this comment.
I'm a bit concerned that this indicates a breaking change happened in the resource manager client library. Please find the change that broke dictionaries. We might need to update our dependencies in setup.py and the constraints files if this breaking change was intended.
bigframes/clients.py
Outdated
| } # Use a dictionary to avoid problematic google.iam namespace package. | ||
| new_binding = google.iam.v1.policy_pb2.Binding( | ||
| role=role, members=[service_account] | ||
| ) # Use a dictionary to avoid problematic google.iam namespace package. |
There was a problem hiding this comment.
This comment is now out-of-date.
| ) # Use a dictionary to avoid problematic google.iam namespace package. | |
| ) |
Could you try this in google3? I wonder if a //google/iam/v1:iam_policy_py_pb2 dependency was missing last time I tried it?
There was a problem hiding this comment.
Trying.. but that one should be indirectly included via resourcemanager_v3 (which does include it)?
product-auto-label
bot
added
size: s
I tested that even with google-cloud-resource-manager==1.12.0 (more than an year old) the dictionary update does not work. So the break happened with the proto->dictionary change. It is a case of missing test coverage for the IAM update code path (which is not integration tested due to internal test project policies where IAM permissions are managed via config). I am adding a mocked unit test which covers at least the binding update part. |
shobsi
changed the title
shobsi
changed the title
This is needed as some recent updates to a dependency (google-cloud-resource-manager or its dependencies) broke the existing logic, and we are seeing the error like this:
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes internal issues 419589974, 425442503 🦕