refactor(iam): move iam_v2_policy_protos to a separate lib #12413

scotthart · 2023-08-18T19:35:36Z

Create a new library, iam_policy_protos, (similar to how grafeas is treated) to contain the v2 iam policy protos that are needed by multiple services.

The v1 iam policy protos libs are currently built as part of the "always built" proto libraries found in external/googleapis/CMakeLists.txt, but relied upon the generated iam_policy.list file to provide the path to the .proto files. This PR moves those proto path defintions to the external/googleapis/CMakeLists.txt where the libs are defined in order to clearly separate how these protos and libs are defined. Hopefully, in the future, we can factor out more of the proto libs defined in external/googleapis/CMakeLists.txt and in the process combine the v1 and v2 iam policy protos into one iam_policy_protos lib.

This change is

coryan

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scotthart)

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

    google_cloud_cpp_iam_v1_options_protos
    google_cloud_cpp_iam_v1_policy_protos
    google_cloud_cpp_iam_v2_deny_protos

This will let you introduce the new policy troubleshooter service, but will always compile the IAM v2 protos, even when not needed (see #8022). Though I gave up on fixing the existing problem, I would like to not make it bigger.

See what we did for grafeas for inspiration.

codecov · 2023-08-18T20:34:27Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (cc50dd4) 93.61% compared to head (479fb94) 93.62%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #12413   +/-   ##
=======================================
  Coverage   93.61%   93.62%           
=======================================
  Files        2039     2039           
  Lines      180677   180677           
=======================================
+ Hits       169148   169151    +3     
+ Misses      11529    11526    -3

see 5 files with indirect coverage changes

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

coryan

A few things to clean up.

The library name ... I don't have great suggestions but maybe you do?

coryan · 2023-08-22T18:33:16Z

cmake/GoogleCloudCppFeatures.cmake

    if (pubsublite IN_LIST enabled_features)
        list(INSERT enabled_features 0 pubsub)
    endif ()
    if (pubsub IN_LIST enabled_features)
-        list(INSERT enabled_features 0 iam)
+        list(INSERT enabled_features 0 iam iam_policy)


Suggested change

list(INSERT enabled_features 0 iam iam_policy)

list(INSERT enabled_features 0 iam_policy iam)

I think we want to compile iam_policy before compiling iam?

coryan · 2023-08-22T18:36:12Z

google/cloud/iam/CMakeLists.txt

@@ -53,7 +53,8 @@ target_include_directories(
 target_link_libraries(
    google_cloud_cpp_iam
    PUBLIC google-cloud-cpp::grpc_utils google-cloud-cpp::common
-           google-cloud-cpp::iam_protos google-cloud-cpp::iam_v1_policy_protos)
+           google-cloud-cpp::iam_protos google-cloud-cpp::iam_v1_policy_protos
+           google_cloud_cpp_iam_policy_protos)


For consistency, maybe we should use the aliased name?

Suggested change

google_cloud_cpp_iam_policy_protos)

google-cloud-cpp::iam_policy_protos)

coryan · 2023-08-22T18:36:26Z

external/googleapis/update_libraries.sh

-  ["iam_policy"]="$(
-    printf ",%s" \
-      "@com_google_googleapis//google/iam/v1:iam_cc_grpc" \
-      "@com_google_googleapis//google/iam/v1/logging:logging_cc_grpc"


Looks like lost the iam/v1/logging protos? I am not sure it was a good idea to include them in the past, but since we did, we need to keep them.

coryan · 2023-08-22T18:38:24Z

google/cloud/iam_policy/CMakeLists.txt

+            NAMELINK_SKIP
+    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+            COMPONENT google_cloud_cpp_development)
+# With CMake-3.12 and higher we could avoid this separate command (and the
+# duplication).
+install(
+    TARGETS google_cloud_cpp_iam_policy_protos
+    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+            COMPONENT google_cloud_cpp_development
+            NAMELINK_ONLY
+    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
+            COMPONENT google_cloud_cpp_development)


We have not cleaned this up everwhere, but we can do better now that we require CMake >= 3.13:

Suggested change

NAMELINK_SKIP

ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}

COMPONENT google_cloud_cpp_development)

# With CMake-3.12 and higher we could avoid this separate command (and the

# duplication).

install(

TARGETS google_cloud_cpp_iam_policy_protos

LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}

COMPONENT google_cloud_cpp_development

NAMELINK_ONLY

ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}

COMPONENT google_cloud_cpp_development)

NAMELINK_COMPONENT google_cloud_cpp_development

ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}

COMPONENT google_cloud_cpp_development)

coryan · 2023-08-22T18:49:51Z

external/googleapis/protolists/iam_policy.list

-@com_google_googleapis//google/iam/v1:iam_policy.proto
-@com_google_googleapis//google/iam/v1:options.proto
-@com_google_googleapis//google/iam/v1:policy.proto
+@com_google_googleapis//google/iam/v2:deny.proto


The name of the library and its contents do not really match anymore. I think we got here through a series of reasonable decisions, but having a library called iam_policy which only contains the v2 protos seems ... unfortunate.

Would iam_v2 be a better name? or even iam_v2_protos?

scotthart

Reviewable status: 2 of 15 files reviewed, 6 unresolved discussions (waiting on @coryan)

cmake/GoogleCloudCppFeatures.cmake line 207 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I think we want to compile iam_policy before compiling iam?

Done.

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

Previously, coryan (Carlos O'Ryan) wrote…

This will let you introduce the new policy troubleshooter service, but will always compile the IAM v2 protos, even when not needed (see #8022). Though I gave up on fixing the existing problem, I would like to not make it bigger.

See what we did for grafeas for inspiration.

Done.

external/googleapis/update_libraries.sh line 0 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

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

Looks like lost the iam/v1/logging protos? I am not sure it was a good idea to include them in the past, but since we did, we need to keep them.

While the logging protos were included in the iam_policy.list and iam_policy.deps files, no libraries were ever built using them as inputs. So removing the iam/v1/logging protos doesn't affect anything as far as I can tell.

external/googleapis/protolists/iam_policy.list line 1 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

The name of the library and its contents do not really match anymore. I think we got here through a series of reasonable decisions, but having a library called iam_policy which only contains the v2 protos seems ... unfortunate.

Would iam_v2 be a better name? or even iam_v2_protos?

I'm hoping that we can eventually factor out the iam v1 policy protos from external/googleapis/CMakeLists.txt and add them to this new lib, making it whole.

google/cloud/iam/CMakeLists.txt line 57 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

For consistency, maybe we should use the aliased name?
           google-cloud-cpp::iam_policy_protos)

Done.

google/cloud/iam_policy/CMakeLists.txt line 66 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

We have not cleaned this up everwhere, but we can do better now that we require CMake >= 3.13:
            NAMELINK_COMPONENT google_cloud_cpp_development
    ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
            COMPONENT google_cloud_cpp_development)

Done.

dbolduc · 2023-08-24T18:58:07Z

google/cloud/iam/CMakeLists.txt

-    TARGETS google_cloud_cpp_iam
-    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
-            COMPONENT google_cloud_cpp_development
-            NAMELINK_ONLY


Can't say I know what it does, but I think we want:
NAMELINK_COMPONENT google_cloud_cpp_development

dbolduc · 2023-08-24T18:59:04Z

google/cloud/iam_policy/CMakeLists.txt

+    RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+            COMPONENT google_cloud_cpp_runtime
+    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
+            COMPONENT google_cloud_cpp_runtime


scotthart

Reviewable status: 2 of 15 files reviewed, 8 unresolved discussions (waiting on @coryan and @dbolduc)

google/cloud/iam/CMakeLists.txt line 0 at r3 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

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

Can't say I know what it does, but I think we want:
NAMELINK_COMPONENT google_cloud_cpp_development

From what I grok of https://cmake.org/cmake/help/latest/command/install.html NAMELINK_COMPONENT is only needed if we want a different name for the symbolic link.

coryan

Reviewed 10 of 13 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dbolduc and @scotthart)

google/cloud/iam/CMakeLists.txt line 0 at r3 (raw file):

Previously, scotthart (Scott Hart) wrote…

From what I grok of https://cmake.org/cmake/help/latest/command/install.html NAMELINK_COMPONENT is only needed if we want a different name for the symbolic link.

We define our install components to achieve this:

# This will install all the artifacts needed to create the development package of our libraries, i.e., headers, CMake and pkg-config files, as well as .so and necessary .so links 
cmake --install .build --component google_cloud_cpp_development

# This will install all the artifacts needed to create the runtime package of our libraries, just the minimal .so files:
cmake --install .build --component google_cloud_cpp_runtime

NAMELINK_COMPONENT picks which of the "install components" creates links for the .so files. Not the name of the links, but the name of the install component that will create the links.

google/cloud/iam/CMakeLists.txt line 131 at r3 (raw file):

            COMPONENT google_cloud_cpp_runtime
    LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
            COMPONENT google_cloud_cpp_runtime

This is missing the NAMELINK_COMPONENT.

google/cloud/iam_policy/CMakeLists.txt line 66 at r2 (raw file):

Previously, scotthart (Scott Hart) wrote…

Done.

Still missing the NAMELINK_COMPONENT here too.

scotthart

Reviewable status: 13 of 15 files reviewed, 5 unresolved discussions (waiting on @coryan and @dbolduc)

google/cloud/iam/CMakeLists.txt line 131 at r3 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

This is missing the NAMELINK_COMPONENT.

Added.

google/cloud/iam_policy/CMakeLists.txt line 66 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Still missing the NAMELINK_COMPONENT here too.

Added.

coryan

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dbolduc and @scotthart)

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

product-auto-label bot added the api: iam Issues related to the Identity and Access Management API. label Aug 18, 2023

coryan reviewed Aug 18, 2023

View reviewed changes

scotthart temporarily deployed to internal August 21, 2023 20:18 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 21, 2023 20:39 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 15:10 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 15:37 — with GitHub Actions Inactive

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

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

scotthart temporarily deployed to internal August 22, 2023 18:15 — with GitHub Actions Inactive

scotthart force-pushed the refactor_iam_v2_policy_protos branch from ab586e6 to ce905d1 Compare August 22, 2023 18:28

scotthart marked this pull request as ready for review August 22, 2023 18:28

scotthart requested a review from a team as a code owner August 22, 2023 18:28

scotthart temporarily deployed to internal August 22, 2023 18:28 — with GitHub Actions Inactive

coryan suggested changes Aug 22, 2023

View reviewed changes

scotthart temporarily deployed to internal August 22, 2023 18:53 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 20:45 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 20:53 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 21:49 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 22:27 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 22, 2023 23:25 — with GitHub Actions Inactive

scotthart temporarily deployed to internal August 24, 2023 14:51 — with GitHub Actions Inactive

scotthart commented Aug 24, 2023

View reviewed changes

dbolduc reviewed Aug 24, 2023

View reviewed changes

scotthart commented Aug 24, 2023

View reviewed changes

coryan suggested changes Aug 28, 2023

View reviewed changes

scotthart temporarily deployed to internal August 28, 2023 22:52 — with GitHub Actions Inactive

scotthart commented Aug 28, 2023

View reviewed changes

coryan approved these changes Aug 28, 2023

View reviewed changes

scotthart force-pushed the refactor_iam_v2_policy_protos branch from bfa8ce5 to 3174875 Compare August 28, 2023 23:01

scotthart temporarily deployed to internal August 28, 2023 23:01 — with GitHub Actions Inactive

scotthart added 9 commits August 28, 2023 19:05

refactor(iam): move iam_v2_policy_protos to a separate lib

dcabadc

use alias in cmake

618e437

fix dep order

c239a41

correct alias name

5bdcc04

doxygen fixes

2728356

remove older cmake install command

279d2b4

add back cmake install command

faff593

more cmake tweaks

199c107

add correct NAMELINK_COMPONENT directives

32efb61

scotthart force-pushed the refactor_iam_v2_policy_protos branch from 3174875 to 32efb61 Compare August 28, 2023 23:05

scotthart temporarily deployed to internal August 28, 2023 23:05 — with GitHub Actions Inactive

fix rebase merge

479fb94

scotthart temporarily deployed to internal August 28, 2023 23:30 — with GitHub Actions Inactive

scotthart enabled auto-merge (squash) August 28, 2023 23:58

scotthart merged commit 4d0271b into googleapis:main Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(iam): move iam_v2_policy_protos to a separate lib #12413

refactor(iam): move iam_v2_policy_protos to a separate lib #12413

scotthart commented Aug 18, 2023 •

edited

Loading

coryan left a comment

codecov bot commented Aug 18, 2023 •

edited

Loading

coryan left a comment

coryan Aug 22, 2023

coryan Aug 22, 2023

coryan Aug 22, 2023

coryan Aug 22, 2023

coryan Aug 22, 2023

coryan Aug 22, 2023

scotthart left a comment

dbolduc Aug 24, 2023

dbolduc Aug 24, 2023

scotthart left a comment

coryan left a comment

scotthart left a comment

coryan left a comment

	list(INSERT enabled_features 0 iam iam_policy)
	list(INSERT enabled_features 0 iam_policy iam)

	google_cloud_cpp_iam_policy_protos)
	google-cloud-cpp::iam_policy_protos)

refactor(iam): move iam_v2_policy_protos to a separate lib #12413

refactor(iam): move iam_v2_policy_protos to a separate lib #12413

Conversation

scotthart commented Aug 18, 2023 • edited Loading

coryan left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

coryan 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

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

scotthart left a comment

Choose a reason for hiding this comment

coryan left a 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 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading