feat(policytroubleshooter): v3 PolicyTroubleshooter service added #12403

scotthart · 2023-08-17T16:49:40Z

This change is

codecov · 2023-08-17T17:08:18Z

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (80e0a05) 93.62% compared to head (f19f6c4) 93.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12403      +/-   ##
==========================================
- Coverage   93.62%   93.62%   -0.01%     
==========================================
  Files        2043     2043              
  Lines      180841   180841              
==========================================
- Hits       169315   169314       -1     
- Misses      11526    11527       +1

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

coryan · 2023-08-18T13:24:43Z

external/googleapis/CMakeLists.txt

@@ -230,6 +230,8 @@ set(external_googleapis_installed_libraries_list
    google_cloud_cpp_iam_v1_iam_policy_protos
    google_cloud_cpp_iam_v1_options_protos
    google_cloud_cpp_iam_v1_policy_protos
+    google_cloud_cpp_iam_v2_deny_protos


I think google/cloud/iam is already compiling these protos. I believe this change introduces a second library that compiles the same protos. That is basically an ODR violation waiting to happen.

If I am right, we need to refactor these common protos to a new library or we need to have google/cloud/policytroubleshooter depend on google/cloud/iam. The first option seems more desirable, and it would be similar to the pure-protos library in google/cloud/grafeas.

This change makes it so that we always build the v2 protos.

I think we can clean it up as part of #8022.

scotthart

Reviewable status: 0 of 70 files reviewed, 1 unresolved discussion (waiting on @coryan)

external/googleapis/CMakeLists.txt line 233 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I think google/cloud/iam is already compiling these protos. I believe this change introduces a second library that compiles the same protos. That is basically an ODR violation waiting to happen.

If I am right, we need to refactor these common protos to a new library or we need to have google/cloud/policytroubleshooter depend on google/cloud/iam. The first option seems more desirable, and it would be similar to the pure-protos library in google/cloud/grafeas.

So I can put them in a new common lib google_cloud_cpp_iam_v2_policy_protos and make iam depend on that new lib, but external/googleapis/update_libraries.sh adds both

@com_google_googleapis//google/iam/v2:deny.proto
@com_google_googleapis//google/iam/v2:policy.proto

to the iam.list file which returns us to having the protos compiled in two places.

Thoughts?

coryan · 2023-08-18T18:32:58Z

external/googleapis/CMakeLists.txt line 233 at r1 (raw file):

Previously, scotthart (Scott Hart) wrote…

So I can put them in a new common lib google_cloud_cpp_iam_v2_policy_protos and make iam depend on that new lib, but external/googleapis/update_libraries.sh adds both
@com_google_googleapis//google/iam/v2:deny.proto
@com_google_googleapis//google/iam/v2:policy.proto
to the iam.list file which returns us to having the protos compiled in two places.

Thoughts?

I think you would need to remove this line in update_libraries.sh :

google-cloud-cpp/external/googleapis/update_libraries.sh

Line 158 in a297a1a

"@com_google_googleapis//google/iam/v2:iam_cc_grpc"

You would also need to change the iam CMake file to link your new common library.

And you would need to change the CMake files to treat this common library as a dependency of iam.

coryan · 2023-08-18T18:42:30Z

external/googleapis/CMakeLists.txt line 233 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I think you would need to remove this line in update_libraries.sh :

google-cloud-cpp/external/googleapis/update_libraries.sh

Line 158 in a297a1a

"@com_google_googleapis//google/iam/v2:iam_cc_grpc"

You would also need to change the iam CMake file to link your new common library.

And you would need to change the CMake files to treat this common library as a dependency of iam.

FWIW, I think it would be easier (and cleaner) if you made these changes in a PR preparing for this new library.

dbolduc · 2023-08-18T19:16:59Z

google/cloud/policytroubleshooter/iam/v3/.repo-metadata.json

+    "language": "cpp",
+    "library_type": "GAPIC_AUTO",
+    "name_pretty": "Policy Troubleshooter API",
+    "product_documentation": "https://cloud.google.com/policy-intelligence/docs/troubleshoot-access",


This link seems to indicate that the feature is "pre-GA" 🙄

do we need to add an experimental: true field to the generator config? or not generate it at all...

It went GA this week. There may be a rollout delay on the documentation.

dbolduc

Sorry, looks like #12487 won the race

dbolduc · 2023-08-29T16:54:49Z

google/cloud/policytroubleshooter/config.cmake.in

@@ -14,6 +14,7 @@

 include(CMakeFindDependencyMacro)
 # google_cloud_cpp_googleapis finds both gRPC and Protobuf, no need to load them here.
+find_dependency(google_cloud_cpp_iam_policy)


I just deleted this file out from under you...

dbolduc · 2023-08-29T16:57:03Z

google/cloud/policytroubleshooter/CMakeLists.txt

+set(GOOGLE_CLOUD_CPP_DOXYGEN_EXTRA_INCLUDES
+    "${PROJECT_BINARY_DIR}/google/cloud/iam_policy")

 google_cloud_cpp_add_ga_grpc_library(policytroubleshooter
                                     "Policy Troubleshooter API")


This should handle the config.cmake.in and the DOXYGEN_EXTRA_INCLUDES:

google_cloud_cpp_add_ga_grpc_library(policytroubleshooter "Policy Troubleshooter API" CROSS_LIB_DEPS "iam_policy")

scotthart

Reviewable status: 0 of 75 files reviewed, 4 unresolved discussions (waiting on @coryan and @dbolduc)

a discussion (no related file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

I just deleted this file out from under you...

Rebased.

external/googleapis/CMakeLists.txt line 233 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

FWIW, I think it would be easier (and cleaner) if you made these changes in a PR preparing for this new library.

Done.

google/cloud/policytroubleshooter/CMakeLists.txt line 25 at r3 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

This should handle the config.cmake.in and the DOXYGEN_EXTRA_INCLUDES:

google_cloud_cpp_add_ga_grpc_library(policytroubleshooter
                                     "Policy Troubleshooter API"
                                     CROSS_LIB_DEPS "iam_policy")

Done.

dbolduc · 2023-08-29T17:27:07Z

google/cloud/policytroubleshooter/CMakeLists.txt

+set(GOOGLE_CLOUD_CPP_DOXYGEN_EXTRA_INCLUDES
+    "${PROJECT_BINARY_DIR}/google/cloud/iam_policy")


can remove this. It is handled by CROSS_LIB_DEPS

dbolduc · 2023-08-29T17:27:24Z

cmake/templates/config.cmake.in

@@ -1,23 +0,0 @@
-# Copyright 2023 Google LLC


dbolduc · 2023-08-29T17:32:52Z

cmake/templates/config.cmake.in

@@ -14,6 +14,7 @@

 include(CMakeFindDependencyMacro)
 # google_cloud_cpp_googleapis finds both gRPC and Protobuf, no need to load them here.
+find_dependency(google_cloud_cpp_iam_policy)


remove this.

scotthart

Reviewable status: 0 of 75 files reviewed, 6 unresolved discussions (waiting on @coryan and @dbolduc)

cmake/templates/config.cmake.in line 0 at r5 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

uh oh...

hopefully correctly reverted this merge consequence

coryan

Reviewed 63 of 70 files at r1, 9 of 11 files at r3, 1 of 3 files at r4, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dbolduc and @scotthart)

scotthart requested a review from a team as a code owner August 17, 2023 16:49

scotthart temporarily deployed to internal August 17, 2023 16:49 — with GitHub Actions Inactive

product-auto-label bot added the api: policytroubleshooter Issues related to the IAM Policy Troubleshooter API API. label Aug 17, 2023

scotthart force-pushed the feat_policytroubleshooter branch from ad66582 to 16118b7 Compare August 17, 2023 17:59

scotthart temporarily deployed to internal August 17, 2023 17:59 — with GitHub Actions Inactive

coryan suggested changes Aug 18, 2023

View reviewed changes

scotthart commented Aug 18, 2023

View reviewed changes

scotthart temporarily deployed to internal August 18, 2023 19:09 — with GitHub Actions Inactive

dbolduc reviewed Aug 18, 2023

View reviewed changes

scotthart force-pushed the feat_policytroubleshooter branch from 7888378 to 16118b7 Compare August 29, 2023 14:57

scotthart temporarily deployed to internal August 29, 2023 14:57 — with GitHub Actions Inactive

scotthart force-pushed the feat_policytroubleshooter branch from 16118b7 to 2e0e992 Compare August 29, 2023 16:41

scotthart temporarily deployed to internal August 29, 2023 16:41 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 29, 2023 16:55 — with GitHub Actions Inactive

dbolduc reviewed Aug 29, 2023

View reviewed changes

scotthart added 4 commits August 29, 2023 13:18

feat(policytroubleshooter): v3 PolicyTroubleshooter service added

2dda00f

generated code

a061738

cmake fixes

f1845ab

regenerate

48ec918

scotthart force-pushed the feat_policytroubleshooter branch from c465f22 to 1792b2a Compare August 29, 2023 17:18

scotthart temporarily deployed to internal August 29, 2023 17:18 — with GitHub Actions Inactive

scotthart commented Aug 29, 2023

View reviewed changes

scotthart temporarily deployed to internal August 29, 2023 17:26 — with GitHub Actions Inactive

dbolduc reviewed Aug 29, 2023

View reviewed changes

cmake

2c8604b

scotthart force-pushed the feat_policytroubleshooter branch from ff5fd5b to 2c8604b Compare August 29, 2023 17:31

scotthart temporarily deployed to internal August 29, 2023 17:32 — with GitHub Actions Inactive

dbolduc reviewed Aug 29, 2023

View reviewed changes

scotthart commented Aug 29, 2023

View reviewed changes

remove iam_policy

f19f6c4

scotthart temporarily deployed to internal August 29, 2023 17:50 — with GitHub Actions Inactive

coryan approved these changes Aug 30, 2023

View reviewed changes

scotthart merged commit 7212399 into googleapis:main Aug 30, 2023
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(policytroubleshooter): v3 PolicyTroubleshooter service added #12403

feat(policytroubleshooter): v3 PolicyTroubleshooter service added #12403

scotthart commented Aug 17, 2023 •

edited by coryan

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

coryan Aug 18, 2023

dbolduc Aug 30, 2023

scotthart left a comment

coryan commented Aug 18, 2023

coryan commented Aug 18, 2023

dbolduc Aug 18, 2023 •

edited

Loading

scotthart Aug 18, 2023

dbolduc left a comment

dbolduc Aug 29, 2023

dbolduc Aug 29, 2023

scotthart left a comment

dbolduc Aug 29, 2023

dbolduc Aug 29, 2023

dbolduc Aug 29, 2023

dbolduc Aug 29, 2023

scotthart left a comment

coryan left a comment

		set(GOOGLE_CLOUD_CPP_DOXYGEN_EXTRA_INCLUDES
		"${PROJECT_BINARY_DIR}/google/cloud/iam_policy")

feat(policytroubleshooter): v3 PolicyTroubleshooter service added #12403

feat(policytroubleshooter): v3 PolicyTroubleshooter service added #12403

Conversation

scotthart commented Aug 17, 2023 • edited by coryan Loading

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

coryan commented Aug 18, 2023

coryan commented Aug 18, 2023

dbolduc Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolduc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotthart left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

scotthart commented Aug 17, 2023 •

edited by coryan

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

dbolduc Aug 18, 2023 •

edited

Loading