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

chore: bake synthtool and owl-bot into library_generation docker image #2615

Merged
merged 41 commits into from
May 1, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Apr 2, 2024

Context

Milestone 7 of Hermetic Build

In this PR

  • Bake synthtool into the docker image (we won't git clone it anymore)
  • Bake the owlbot CLI into the docker image (we won't use docker inside docker anymore)
  • Create a DEVELOPMENT.md file with instructions for local setup (example: install owlbot CLI locally)

Follow ups

  • modify google-cloud-java's workflows to correct the volume mapping being used

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Apr 2, 2024
@diegomarquezp diegomarquezp changed the title chore: bake synthtool into library_generation docker image chore: bake synthtool and owl-bot into library_generation docker image Apr 2, 2024
@diegomarquezp diegomarquezp changed the title chore: bake synthtool and owl-bot into library_generation docker image chore: [wip] bake synthtool and owl-bot into library_generation docker image Apr 2, 2024
@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 Apr 11, 2024
@JoeWang1127
Copy link
Collaborator

We also need to modify workflow files in google-cloud-java after this pull request is merged because the docker command is changed.

@JoeWang1127
Copy link
Collaborator

JoeWang1127 commented Apr 22, 2024

[Maybe out of scope of this PR] postprocess_library.sh is ~100 lines in this PR, should we refactor it to python script?

RUN chmod -R a+rx /home/.nvm

WORKDIR /workspace
ENTRYPOINT [ "python", "/src/cli/entry_point.py", "generate" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the user run the image with different scripts/methods?

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, I declared this ENTRYPOINT since it's the "official" one. If we want to run a different script, we can pass the --entrypoint /src/my_script.py flag


# set dummy git credentials for the empty commit used in postprocessing
# we use system so all users using the container will use this configuration
RUN git config --system user.email "[email protected]"
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 only reason we need git config is from the post processing here? If it is, I think we should look into not initialize a Git repo, as a follow up enhancement though.

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, but the root cause is that the owl-bot CLI assumes the source folder (the one we generate and arrange) is a git repository. Extra points to process owlbot yamls using a custom script.


# install synthtool
WORKDIR /tools
RUN git clone https://github.com/googleapis/synthtool
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 only reason we need synthtool is java.py and its dependencies?

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, mainly the common_templates function. We could in theory move java.py and that function to sdk-platform

library_generation/DEVELOPMENT.md Show resolved Hide resolved
library_generation/DEVELOPMENT.md Outdated Show resolved Hide resolved
library_generation/DEVELOPMENT.md Show resolved Hide resolved
RUN apt-get update && apt-get install -y \
unzip openjdk-17-jdk rsync maven jq \
unzip openjdk-17-jdk rsync maven jq less vim \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see less and vim being used, I guess the reason we need them is for testing purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just for inspecting the container files in case things go wrong during local development. Do we want to only keep the production dependencies?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For local development, I think you can mention them in the development.md. No need to install it 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.

Makes sense, I removed these tools from the image and added a small section in the development guide.

@blakeli0 blakeli0 requested review from suztomo and chingor13 April 26, 2024 20:11
@diegomarquezp
Copy link
Contributor Author

We also need to modify workflow files in google-cloud-java after this pull request is merged because the docker command is changed.

Yes, indeed. This is considered in the follow up section of the PR description. I will do it right after we merge this

@diegomarquezp
Copy link
Contributor Author

[Maybe out of scope of this PR] postprocess_library.sh is ~100 lines in this PR, should we refactor it to python script?

Yes, my only concern is that xtrace is really useful for debugging the shell scripts. I may give it a try as well as including different levels of logging for some of the scripts

Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

RUN apt-get update && apt-get install -y \
unzip openjdk-17-jdk rsync maven jq \
&& apt-get clean

COPY library_generation /src

# use python 3.11 (the base image has several python versions; here we define the default one)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the base image cloud-devrel-public-resources still uses 3.11 so we have to use 3.11? Who maintains cloud-devrel-public-resources?

Copy link
Contributor Author

@diegomarquezp diegomarquezp May 1, 2024

Choose a reason for hiding this comment

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

so we have to use 3.11?

We could use a higher version too, or even a lower one if it's still compatible with the dependencies

who maintains cloud-devrel-public-resources?

Judging from a documentation entry in the cloud client libraries g3doc, I think it's the Cloud Client Libraries team
Maybe we can extract the base Dockerfile and prepend it to ours to eliminate this dependency.

@diegomarquezp diegomarquezp merged commit 16aec03 into main May 1, 2024
44 checks passed
@diegomarquezp diegomarquezp deleted the transfer-synthtool-java branch May 1, 2024 17:14
diegomarquezp added a commit to googleapis/google-cloud-java that referenced this pull request May 13, 2024
diegomarquezp added a commit to googleapis/google-cloud-java that referenced this pull request May 13, 2024
lqiu96 pushed a commit that referenced this pull request May 22, 2024
In this PR:
- Remove generated files after running tests.

Note that files generated by `docker run` have root permission, thus
can't be removed by this PR. This issue will be resolved in #2615.
lqiu96 pushed a commit that referenced this pull request May 22, 2024
#2615)

## Context
[Milestone 7 of Hermetic
Build](https://docs.google.com/document/d/1v-sJBmdNEVBRryL8n90XK8OIiqVphaJemXVhANw14Jk/edit?pli=1&resourcekey=0-QGUb2do-JBlDWKvptWBp_g&tab=t.0#bookmark=id.bkbj04ib2d4n)

## In this PR
- Bake synthtool into the docker image (we won't `git clone` it anymore)
- Bake the owlbot CLI into the docker image (we won't use docker inside
docker anymore)
- Create a DEVELOPMENT.md file with instructions for local setup
(example: install owlbot CLI locally)

## Follow ups
- [ ] modify google-cloud-java's workflows to correct the volume mapping
being used

---------

Co-authored-by: Joe Wang <[email protected]>
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