Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: [kmsinventory] new module for kmsinventory #9162

Merged
merged 5 commits into from
Feb 28, 2023
Merged

feat: [kmsinventory] new module for kmsinventory #9162

merged 5 commits into from
Feb 28, 2023

Conversation

emmileaf
Copy link
Contributor

@emmileaf emmileaf commented Feb 27, 2023 •
edited

Initial client library generation for kmsinventory:

python3.9 generation/new_client/new-client.py generate \
  --api_shortname=kmsinventory \
  --proto-path=google/cloud/kms/inventory \
  --name-pretty="KMS Inventory API" \
  --product-docs="https://cloud.google.com/kms/docs/" \
  --api-description="KMS Inventory API."

Note: there is no DevRel Services product page for this API yet (hence the failing snippetbot checks), and parameters above were collected from ticket (pointing to https://github.com/googleapis/googleapis/blob/master/google/cloud/kms/inventory/v1/kmsinventory_v1.yaml).

Summary of manual adjustments:

  • Added dependency on proto-google-cloud-kms-v1: 98daccc and 5fe62af
  • Added version update annotation to readme: afa8738

emmileaf
emmileaf commented Feb 27, 2023
View reviewed changes
...tory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponseOrBuilder.java
*
* <code>repeated .google.cloud.kms.v1.CryptoKey crypto_keys = 1;</code>
*/
java.util.List<com.google.cloud.kms.v1.CryptoKey> getCryptoKeysList();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compilation error (logs), from dependency on com.google.cloud.kms.v1

Error:  Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.10.1:compile (default-compile) on project proto-google-cloud-kmsinventory-v1: Compilation failure: Compilation failure: 
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponseOrBuilder.java:[35,41] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponseOrBuilder.java:[45,26] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponseOrBuilder.java:[65,51] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponseOrBuilder.java:[75,26] package com.google.cloud.kms.v1 does not exist

...-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java
public static final int CRYPTO_KEYS_FIELD_NUMBER = 1;

@SuppressWarnings("serial")
private java.util.List<com.google.cloud.kms.v1.CryptoKey> cryptoKeys_;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compilation error (logs), from dependency on com.google.cloud.kms.v1

Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[75,49] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[86,48] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[99,58] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[126,33] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[139,33] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[612,51] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[623,36] package com.google.cloud.kms.v1 does not exist
Error:  /home/runner/work/google-cloud-java/google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/src/main/java/com/google/cloud/kms/inventory/v1/ListCryptoKeysResponse.java:[624,46] package com.google.cloud.kms.v1.CryptoKey does not exist
...

blakeli0
blakeli0 reviewed Feb 27, 2023
View reviewed changes
java-kmsinventory/pom.xml
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-cloud-kmsinventory-v1</artifactId>
<version>0.0.1-SNAPSHOT</version><!-- {x-version-update:proto-google-cloud-kmsinventory-v1:current} -->
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to add a section like this to here:

  <groupId>com.google.api.grpc</groupId>
  <artifactId>proto-google-cloud-kms-v1</artifactId>
  <version>0.106.0-SNAPSHOT</version><!-- {x-version-update:proto-google-cloud-kms-v1:current} -->

to fix the compilation issue.
@suztomo @meltsufin FYI, this is a rare case that an API is dependent on another API's proto, I think it should be fine as long as the versions are centrally maintained in versions.txt, but want to make sure this is not a problem for releasing.

@lqiu96
Copy link
Contributor

lqiu96 commented Feb 27, 2023 •
edited

I believe there was an older thread regarding this library using cross-api dependencies: https://chat.google.com/room/AAAAYlCaXjg/sqzmXYYnbX8

Blake mentioned that asset already does this and maybe we can do something similar:

google-cloud-java/java-asset/pom.xml

Lines 84 to 93 in 5fd6f61

<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-cloud-orgpolicy-v1</artifactId>
<version>2.12.0-SNAPSHOT</version><!-- {x-version-update:proto-google-cloud-orgpolicy-v1:current} -->
</dependency>
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-identity-accesscontextmanager-v1</artifactId>
<version>1.13.0-SNAPSHOT</version><!-- {x-version-update:proto-google-identity-accesscontextmanager-v1:current} -->
</dependency>

Er, Blake beat me to it... I believe it's the same idea.

@emmileaf
Copy link
Contributor Author

emmileaf commented Feb 27, 2023 •
edited

@blakeli0 @lqiu96 Ah sorry I just saw your comments after pushing f181a73! Would adding a dependency on proto-google-cloud-kms-v1 be preferred here to adding google-cloud-kms as a manual dependency adjustment?

Edit: updated in 98daccc to dependency on proto-google-cloud-kms-v1.

@lqiu96
Copy link
Contributor

lqiu96 commented Feb 27, 2023

@blakeli0 @lqiu96 Ah sorry I just saw your comments after pushing f181a73! Would adding a dependency on proto-google-cloud-kms-v1 be preferred here to adding google-cloud-kms as a manual dependency adjustment?

I think adding the proto module dependency is be preferred (though not sure what everyone else's opinion is). Looks like we're only dependent on CryptoKey in the proto-* module (https://github.com/googleapis/google-cloud-java/blob/5fd6f610f52824b4a25b0b455a4d9503c0de72b8/java-kms/proto-google-cloud-kms-v1/src/main/java/com/google/cloud/kms/v1/CryptoKey.java).

@emmileaf
Copy link
Contributor Author

The checks are hitting the same compilation errors - locally running mvn -f java-kmsinventory compile and mvn compile succeeds, but running the exact command as build.sh fails:

google-cloud-java/.kokoro/build.sh

Lines 41 to 49 in 5fd6f61

mvn -B -ntp \
-Dclirr.skip=true \
-Denforcer.skip=true \
-Dcheckstyle.skip=true \
-Dflatten.skip=true \
-Danimal.sniffer.skip=true \
-Dmaven.wagon.http.retryHandler.count=5 \
-T 1C \
test

Perhaps there's something else needed (for test scope)? Going to experiment a bit more locally.

@emmileaf
Copy link
Contributor Author

Update: ci/unit checks are passing now - thanks for the help @blakeli0 and @lqiu96 !

Manual dependency additions made in 98daccc and 5fe62af:

google-cloud-java/java-kmsinventory/proto-google-cloud-kmsinventory-v1/pom.xml

Lines 36 to 39 in 5fe62af

<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-cloud-kms-v1</artifactId>
</dependency>

google-cloud-java/java-kmsinventory/pom.xml

Lines 44 to 49 in 5fe62af

<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-cloud-kms-v1</artifactId>
<version>0.106.0-SNAPSHOT</version><!-- {x-version-update:proto-google-cloud-kms-v1:current} -->
</dependency>
</dependencies>

@emmileaf emmileaf marked this pull request as ready for review February 27, 2023 22:31
@snippet-bot
Copy link

snippet-bot bot commented Feb 27, 2023 •
edited

Here is the summary of possible violations 😱

There are 26 possible violations for not having product prefix.

The end of the violation section. All the stuff below is FYI purposes only.

Here is the summary of changes.

You are about to add 26 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

blakeli0
blakeli0 reviewed Feb 28, 2023
View reviewed changes
java-kmsinventory/README.md
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-kmsinventory</artifactId>
<version>0.0.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to manually add release please set up for this version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in afa8738, following #9150 (cc/ @ddixit14)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you please add it to our new module creation guide as well? cc @suztomo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blakeli0 I think #9150 (just merged today) automated this step into the new client generation script. This new module PR was generated yesterday morning so it didn't have the changes and needed the manual update.

@emmileaf emmileaf merged commit 0fd61a4 into main Feb 28, 2023
@emmileaf emmileaf deleted the new_module_java-kmsinventory branch February 28, 2023 19:58
This was referenced Feb 28, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeli0 blakeli0 blakeli0 left review comments

@lqiu96 lqiu96 lqiu96 approved these changes

@ddixit14 ddixit14 ddixit14 approved these changes

@suztomo suztomo Awaiting requested review from suztomo

@meltsufin meltsufin Awaiting requested review from meltsufin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@emmileaf @lqiu96 @blakeli0 @ddixit14