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.

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

fix: update generate-library-list.sh for duplicate api_shortnames #2873

Merged
merged 4 commits into from May 15, 2024

Conversation

alicejli
Copy link
Contributor

Generate Spring Auto-Configurations is currently failing on securitycenter's module due it having a duplicate api_shortname (https://github.com/googleapis/google-cloud-java/blob/main/generation_config.yaml#L1666).

This updates the script to get the values needed from generation_config.yaml rather than looping through google-cloud-java folders and mimics the hermetic build script logic for those values. It uses library_name if it exists (which is the unique moniker if there is a duplicate api_shortname).

It removes language as a variable as I didn't see it being used elsewhere in the script.

This also updates the default value of the PR branch name to the correct value.

@alicejli alicejli requested a review from a team as a code owner May 10, 2024 20:11
Copy link
Contributor

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

Is there anyway we can test this logic before merging?

@@ -5,7 +5,7 @@ on:
branch_name:
description: PR branch name
required: true
default: "renovate/main-gcp-libraries-bom.version"
default: "renovate/gcp-libraries-bom.version"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - it's intentional. You can see the correct branch name here:
image
on #2866

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I see that main-gcp-libraries-bom is still referenced on line 20 below, which I think probably needs to be updated as well. Do you mind updating it too?

# Determine distribution_name
distribution_name=$(echo "$config" | jq -r '.distribution_name' // "")
if [ -z "$distribution_name" ]; then
distribution_name="com.google.cloud:google-cloud-${unique_module_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we get the logic from here? It will not work for non-cloud apis, but I think we are not publishing SpringCodeGen for them anyway, so we should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's correct. And yeah I assumed as much, but I've added an explicit comment about that now

Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@alicejli
Copy link
Contributor Author

Is there anyway we can test this logic before merging?

I tested this latest iteration locally and it produced a correct library_list.txt file.

@alicejli alicejli merged commit 035f2c3 into main May 15, 2024
54 of 55 checks passed
@alicejli alicejli deleted the updateGenerateAutoConfigStarterScripts branch May 15, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants