-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
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. ```
This reverts commit c25a14a.
@@ -1,6 +1,6 @@ | |||
#!/usr/bin/env bash | |||
|
|||
set -eo pipefail | |||
set -exo pipefail |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
hermetic_build/DEVELOPMENT.md
Outdated
@@ -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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/**/*", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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. |
There was a problem hiding this comment.
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/**/*", |
There was a problem hiding this comment.
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.
hermetic_build/DEVELOPMENT.md
Outdated
@@ -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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
I don't think CODEOWNERS can be templated any more. |
Can you evaluate the templated, generated files are really needed? Seeing kokoro/nightly/java7.cfg, it seems you haven't checked that. |
hermetic_build/library_generation/owlbot/templates/java_library/.github/generated-files-bot.yml
Outdated
Show resolved
Hide resolved
|
|
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 ofupdate_generation_config
yaml
andsh
files.The fix to include the
.github
folder follows from https://setuptools.pypa.io/en/latest/userguide/datafiles.html:Interestingly, this is also the reason
cp synthool/gcp/templates/java_library/* ...
did not bring folders starting with dot (such as .kokoro) into #2884Confirming effects in downstream repos
Demos show the results as of b66af92 in
There were no regressions on templated files that were manually modified.