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

chore: update managedkafka #11314

Merged
merged 2 commits into from
Nov 18, 2024
Merged

chore: update managedkafka #11314

merged 2 commits into from
Nov 18, 2024

Conversation

JoeWang1127
Copy link
Contributor

@JoeWang1127 JoeWang1127 commented Nov 18, 2024 •
edited
Loading

In #11308, library config of managedkafka is changed.

Update the library using

docker run \
 --rm \
 -u "$(id -u):$(id -g)" \
 -v "$(pwd):${workspace_name}" \
 -v "$(pwd)/../googleapis:${workspace_name}/googleapis" \
 -e GENERATOR_VERSION=2.50.0 \
 gcr.io/cloud-devrel-public-resources/java-library-generation:2.50.0 \
 --generation-config-path="${workspace_name}/generation_config.yaml" \
 --api-definitions-path="${workspace_name}/googleapis" \
 --library-names=managedkafka

@JoeWang1127 JoeWang1127 marked this pull request as ready for review November 18, 2024 14:16
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner November 18, 2024 14:16
@JoeWang1127 JoeWang1127 requested a review from suztomo November 18, 2024 14:16
suztomo
suztomo requested changes Nov 18, 2024
View reviewed changes
Copy link
Member

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

Can you remove "API" from the source of truth line in the YAML?

java-managedkafka/README.md Outdated
@@ -67,7 +67,7 @@ See the [Authentication][authentication] section in the base directory's README.

## Authorization

The client application making API calls must be granted [authorization scopes][auth-scopes] required for the desired Managed Service for Apache Kafka API APIs, and the authenticated principal must have the [IAM role(s)][predefined-iam-roles] required to access GCP resources using the Managed Service for Apache Kafka API calls.
The client application making API calls must be granted [authorization scopes][auth-scopes] required for the desired Managed Service for Apache Kafka API APIs, and the authenticated principal must have the [IAM role(s)][predefined-iam-roles] required to access GCP resources using the Managed Service for Apache Kafka API API calls.
Copy link
Member

Choose a reason for hiding this comment

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

This "API API" tells me that the YAML source shouldn't have "API" in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 2nd API comes from the template, it assumes the name pretty doesn't end with API.

We need to make the change in sdk-platform-java and populate the change in the next release.

Copy link
Member

@suztomo suztomo Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm asking to fix the name pretty value. Not template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JoeWang1127 JoeWang1127 requested a review from suztomo November 18, 2024 15:31
suztomo
suztomo approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

Thank you.

@JoeWang1127 JoeWang1127 merged commit 9801be5 into main Nov 18, 2024
31 checks passed
@JoeWang1127 JoeWang1127 deleted the chore/update-managedkafka branch November 18, 2024 16:23
@blakeli0
Copy link
Contributor

Curious that did you manually generate it from local? Theoretically updating the generation_config.yaml should trigger re-generation automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suztomo suztomo suztomo approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@JoeWang1127 @blakeli0 @suztomo