Skip to content

feat: [kmsinventory] new module for kmsinventory#9162

Merged
emmileaf merged 5 commits into
mainfrom
new_module_java-kmsinventory
Feb 28, 2023
Merged

feat: [kmsinventory] new module for kmsinventory#9162
emmileaf merged 5 commits into
mainfrom
new_module_java-kmsinventory

Conversation

@emmileaf

@emmileaf emmileaf commented Feb 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread ...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();

@emmileaf emmileaf Feb 27, 2023

Copy link
Copy Markdown
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

Comment thread ...-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_;

@emmileaf emmileaf Feb 27, 2023

Copy link
Copy Markdown
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
Comment thread 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>

@blakeli0 blakeli0 Feb 27, 2023

Copy link
Copy Markdown
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

lqiu96 commented Feb 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

emmileaf commented Feb 27, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

lqiu96 commented Feb 27, 2023

Copy link
Copy Markdown
Member

@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

emmileaf commented Feb 27, 2023

Copy link
Copy Markdown
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

emmileaf commented Feb 27, 2023

Copy link
Copy Markdown
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

snippet-bot Bot commented Feb 27, 2023 •
edited
Loading

Copy link
Copy Markdown

Here is the summary of possible violations 😱

Details

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
Comment thread java-kmsinventory/README.md
<dependency>
<groupId>com.google.cloud</groupId>
<artifactId>google-cloud-kmsinventory</artifactId>
<version>0.0.0</version>

@blakeli0 blakeli0 Feb 28, 2023

Copy link
Copy Markdown
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

@emmileaf emmileaf Feb 28, 2023

Copy link
Copy Markdown
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)

@blakeli0 blakeli0 Feb 28, 2023

Copy link
Copy Markdown
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

@emmileaf emmileaf Feb 28, 2023

Copy link
Copy Markdown
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

@suztomo suztomo Awaiting requested review from suztomo

@meltsufin meltsufin Awaiting requested review from meltsufin

+1 more reviewer

@ddixit14 ddixit14 ddixit14 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@emmileaf @lqiu96 @blakeli0 @ddixit14