Skip to content

chore(hermetic-build): include .github template updates as part of generation #3723

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

Merged
merged 14 commits into from
Apr 7, 2025

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Mar 26, 2025

Part of the fix for #3701

Approach

The templates come from https://github.com/googleapis/synthtool/tree/bf182cd41d9a7de56092cafcc7befe6b398332f6. The only update was to include generated-files-bot, which is already up to date in all the HW repos.

The .kokoro folder will be a separate follow up task. We will now focus on solving the update of update_generation_config yaml and sh files.

The fix to include the .github folder follows from https://setuptools.pypa.io/en/latest/userguide/datafiles.html:

Glob patterns do not automatically match dotfiles, i.e., directory or file names starting with a dot (.).
To include such files, you must explicitly start the pattern with a dot, e.g. .* to match .gitignore.

Interestingly, this is also the reason cp synthool/gcp/templates/java_library/* ... did not bring folders starting with dot (such as .kokoro) into #2884

Confirming effects in downstream repos

Demos show the results as of b66af92 in

There were no regressions on templated files that were manually modified.

The templates come from https://github.com/googleapis/synthtool/tree/bf182cd41d9a7de56092cafcc7befe6b398332f6

The fix follows from https://setuptools.pypa.io/en/latest/userguide/datafiles.html:
```
Glob patterns do not automatically match dotfiles, i.e., directory or file names starting with a dot (.).
To include such files, you must explicitly start the pattern with a dot, e.g. .* to match .gitignore.
```
@diegomarquezp diegomarquezp added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 26, 2025
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Mar 26, 2025
@diegomarquezp diegomarquezp added wip Work in Progress and removed size: l Pull request size is large. labels Mar 26, 2025
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Mar 26, 2025
@@ -1,6 +1,6 @@
#!/usr/bin/env bash

set -eo pipefail
set -exo pipefail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

xtrace was the only way I could debug my way out in local development. This decision may have taken longer for someone less familiar with the codebase.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't add -x because I think the extra logs may substantial and this level of logs is not recommended for production.

Do you think it's necessary to add these logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this and explained how this would be useful in the dev guide.

@diegomarquezp diegomarquezp marked this pull request as ready for review March 27, 2025 20:20
@@ -78,7 +78,7 @@ as per [POSIX env var definition](https://pubs.opengroup.org/onlinepubs/96999197
2. Move the jar into its well-known location.

```shell
mv /path/to/jar "${HOME}/.library_generation/gapic-generator-java.jar"
mv ~/.m2/repository/com/google/api/gapic-generator-java/{version}/gapic-generator-java-{version}.jar "${HOME}/.library_generation/gapic-generator-java.jar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this too much detail? What if a user change the default maven dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it may be more convenient for copying the whole line and limit to making a replacement in {version}. I think a non-default maven setup is not a common use case, but still checks out, so I reverted this line.

@@ -130,6 +130,9 @@ owl-bot copy-code --version
The key step is `npm link`, which will make the command available in you current
shell session.

If you get a permission denied error when running the command `owl-bot`, try
relinking owl-bot by running `npm unlink -g` and re-running the steps above.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would rerun the same command can solve the permission issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm link has effect in the current shell session. My guess is that what's in an old session may get its permissions overriden.

@@ -24,6 +24,8 @@
"owlbot/src/poms/*.py",
"owlbot/templates/clirr/*.j2",
"owlbot/templates/poms/*.j2",
"owlbot/templates/java_library/.github/**/*",
"owlbot/templates/java_library/.kokoro/**/*",
"owlbot/templates/java_library/**/*",
Copy link
Collaborator

@JoeWang1127 JoeWang1127 Mar 27, 2025

Choose a reason for hiding this comment

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

Do we still need this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there are files outside .github and .kokoro that this line would include.

@@ -1,6 +1,6 @@
#!/usr/bin/env bash

set -eo pipefail
set -exo pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, should we include extra logs in production?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this and explained how this would be useful in the dev guide.

Copy link
Contributor Author

@diegomarquezp diegomarquezp left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

@@ -130,6 +130,9 @@ owl-bot copy-code --version
The key step is `npm link`, which will make the command available in you current
shell session.

If you get a permission denied error when running the command `owl-bot`, try
relinking owl-bot by running `npm unlink -g` and re-running the steps above.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm link has effect in the current shell session. My guess is that what's in an old session may get its permissions overriden.

@@ -24,6 +24,8 @@
"owlbot/src/poms/*.py",
"owlbot/templates/clirr/*.j2",
"owlbot/templates/poms/*.j2",
"owlbot/templates/java_library/.github/**/*",
"owlbot/templates/java_library/.kokoro/**/*",
"owlbot/templates/java_library/**/*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, there are files outside .github and .kokoro that this line would include.

@@ -78,7 +78,7 @@ as per [POSIX env var definition](https://pubs.opengroup.org/onlinepubs/96999197
2. Move the jar into its well-known location.

```shell
mv /path/to/jar "${HOME}/.library_generation/gapic-generator-java.jar"
mv ~/.m2/repository/com/google/api/gapic-generator-java/{version}/gapic-generator-java-{version}.jar "${HOME}/.library_generation/gapic-generator-java.jar"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it may be more convenient for copying the whole line and limit to making a replacement in {version}. I think a non-default maven setup is not a common use case, but still checks out, so I reverted this line.

@@ -1,6 +1,6 @@
#!/usr/bin/env bash

set -eo pipefail
set -exo pipefail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this and explained how this would be useful in the dev guide.

@@ -1,6 +1,6 @@
#!/usr/bin/env bash

set -eo pipefail
set -exo pipefail
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this and explained how this would be useful in the dev guide.

Copy link
Collaborator

@JoeWang1127 JoeWang1127 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@suztomo
Copy link
Member

suztomo commented Mar 27, 2025

I don't think CODEOWNERS can be templated any more.

@suztomo
Copy link
Member

suztomo commented Mar 27, 2025

Can you evaluate the templated, generated files are really needed? Seeing kokoro/nightly/java7.cfg, it seems you haven't checked that.

@diegomarquezp diegomarquezp removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 27, 2025
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Mar 28, 2025
@diegomarquezp diegomarquezp changed the title chore(hermetic-build): include .kokoro and .github templates chore(hermetic-build): include .github template updates as part of generation Mar 28, 2025
@diegomarquezp diegomarquezp added the automerge Merge the pull request once unit tests and other checks pass. label Apr 4, 2025
Copy link

sonarqubecloud bot commented Apr 4, 2025

Copy link

sonarqubecloud bot commented Apr 4, 2025

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@diegomarquezp diegomarquezp merged commit 1c0841f into main Apr 7, 2025
52 of 53 checks passed
@diegomarquezp diegomarquezp deleted the fix-template-updates branch April 7, 2025 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the pull request once unit tests and other checks pass. size: m Pull request size is medium. wip Work in Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants