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

docs: modify hermetic build docs #3331

Merged
merged 17 commits into from
Nov 5, 2024
Merged

Conversation

JoeWang1127
Copy link
Collaborator

@JoeWang1127 JoeWang1127 commented Oct 31, 2024

In this PR:

  • Add instructions on release note generation.
  • Move generating libraries using docker image to README.md
  • Move generating libraries using python scripts to DEVELOPMENT.md.
  • Move instructions on building image to DEVELOPMENT.md.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Oct 31, 2024

Instead of running docker image, if you prefer running the underlying python
script directly, please refer to [development guide](DEVELOPMENT.md) for
additional instructions.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the purpose of README is to instruct users how to do things in the most convenient way, thus I moved the part of running the script directly to development guide as it requires additional setups that users typically don't bother to do.

Instead, I moved the image building part here because it's the most straightforward way (i.e., no need to install dependencies) of generating a repository.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the differences between README and DEVELOPMENT is that, README is for external users that just want to run our hermetic image as a product, while DEVELOPMENT is for internal developers that try to maintain the scripts and images.
Hence for README users, they can think hermetic code generation as a blackbox, all they need to know is the input and output. For DEVELOPMENT users, they need to understand how the process uses Python/Shell/Docker etc, so build image locally is better suited for DEVELOPMENT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SG, I moved the instructions on building image from source to development guide.

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Nov 1, 2024
@JoeWang1127 JoeWang1127 marked this pull request as ready for review November 1, 2024 16:08
Copy link
Contributor

@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.

LGTM overall. Sorry for so many nits!

hermetic_build/README.md Outdated Show resolved Hide resolved
hermetic_build/README.md Outdated Show resolved Hide resolved
hermetic_build/README.md Outdated Show resolved Hide resolved
hermetic_build/README.md Show resolved Hide resolved
hermetic_build/README.md Outdated Show resolved Hide resolved
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
hermetic_build/README.md Outdated Show resolved Hide resolved
```shell
# Assume you want to generate the library in the current working directory
# and the generation configuration is in the same directory.
docker run \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This command includes all the possible parameters, can we provide an example with minimum parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your original example still makes sense for more advanced use cases, I didn't mean to ask you to delete it, we can still have another example with all the parameters. On the other hand, maybe we don't have to provide a more complex example as it could confuse other users. So either way works for me, feel free to decide as you see fit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added back the advance example with explanations.

@JoeWang1127 JoeWang1127 requested a review from blakeli0 November 4, 2024 21:23
hermetic_build/DEVELOPMENT.md Outdated Show resolved Hide resolved
@@ -129,7 +137,7 @@ They are shared by all GAPICs of a library.
| distribution_name | No | `{group_id}:google-{cloud_prefix}{library_name}` if not specified |
| excluded_poms | No | |
| excluded_dependencies | No | |
| googleapis_commitish | No | use repository level `googleapis_commitish` if not specified. |
| googleapis_commitish | No | use repository level `googleapis_committish` if not specified. |
Copy link
Contributor

@diegomarquezp diegomarquezp Nov 4, 2024

Choose a reason for hiding this comment

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

Suggested change
| googleapis_commitish | No | use repository level `googleapis_committish` if not specified. |
| googleapis_commitish | No | use repository level `googleapis_commitish` if not specified. |

Not the right spelling but it's what's defined in the repo-level section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it should be googleapis_committish according to #3208 (comment).

However, I think the refactor should be in its own PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I misspelled the suggestion string. I was trying to point out that we should stick to googleapis_commitish (one t) since we haven't done such refactor.

hermetic_build/README.md Outdated Show resolved Hide resolved
-u "$(id -u):$(id -g)" \
-v "$(pwd):/workspace" \
-v /path/to/api_definition:/workspace \
-e GENERATOR_VERSION=image-tag \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have to set GENERATOR_VERSION? Is it still used in our scritps? Can we just inline the image-tag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I can see generator version is defined in the generation config example.

I'll remove this param.

@JoeWang1127 JoeWang1127 requested a review from blakeli0 November 4, 2024 22:03
Copy link

sonarcloud bot commented Nov 5, 2024

Copy link

sonarcloud bot commented Nov 5, 2024

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 SonarCloud

@JoeWang1127 JoeWang1127 merged commit 25023af into main Nov 5, 2024
49 checks passed
@JoeWang1127 JoeWang1127 deleted the docs/modify-hermetic-build-docs branch November 5, 2024 16:44
jinseopkim0 pushed a commit that referenced this pull request Nov 14, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.50.0</summary>

##
[2.50.0](v2.49.0...v2.50.0)
(2024-11-14)


### Features

* Add experimental S2A integration in client libraries grpc transport
([#3326](#3326))
([1138ca6](1138ca6))
* enable selective generation based on service config include list
([#3323](#3323))
([0cddadb](0cddadb))
* introduce `java.time` to java-core
([#3330](#3330))
([f202c3b](f202c3b))
* Update Gapic-Generator to generate libraries using `java.time` methods
([#3321](#3321))
([b21c9a4](b21c9a4))


### Bug Fixes

* Fix flaky test
ScheduledRetryingExecutorTest.testCancelOuterFutureAfterStart
([#3335](#3335))
([e73740d](e73740d))
* httpjson callables to trace attempts (started, failed)
([#3300](#3300))
([15a64ee](15a64ee))
* instantiate GaxProperties at build time to ensure we get the protobuf
version
([#3365](#3365))
([bb2a3be](bb2a3be))
* protobuf version not always getting set in headers
([#3322](#3322))
([7f6e470](7f6e470))
* use BuildKit instead of legacy builder to build the Hermetic Build
images
([#3338](#3338))
([222fb45](222fb45))


### Dependencies

* update google auth library dependencies to v1.30.0
([#3367](#3367))
([a31c682](a31c682))
* update grpc dependencies to v1.68.1
([#3240](#3240))
([c8e3941](c8e3941))


### Documentation

* fix list num
([#3356](#3356))
([b7d6296](b7d6296))
* **hermetic-build:** indicate usage of Docker Buildkit in development
guide
([#3337](#3337))
([01e742d](01e742d))
* modify hermetic build docs
([#3331](#3331))
([25023af](25023af))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants