-
Notifications
You must be signed in to change notification settings - Fork 53
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 the grpc plugin in the hermetic library generation image #3045
Conversation
@@ -282,7 +315,7 @@ get_proto_path_from_preprocessed_sources() { | |||
pushd "${sources}" > /dev/null | |||
local proto_library=$(find . -maxdepth 1 -type d -name 'proto-*' | sed 's/\.\///') | |||
local found_libraries=$(echo "${proto_library}" | wc -l) | |||
if [ -z ${proto_library} ]; then | |||
if [[ -z ${proto_library} ]]; then |
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 noticed that the shell unit tests were expecting the binary brackets because proto_library
could be expanded to a string with spaces.
+ '[' -z proto-2 proto-1 ']'
/usr/local/google/home/diegomarquezp/google/sdk-platform-java/library_generation/test/../utils/utilities.sh: line 318: [: proto-2: binary operator expected
fi | ||
echo "$(pwd)/${grpc_filename}" |
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 a debug log?
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.
No, it's the return statement of the function. Many functions in utilities.sh
are made this way. The way we use is variable=$(my_function "arg1" "arg2")
. The only possible return values for return
are integers from 0 to 255 (and consumed via $?
) so it's not useful.
# we indicate protoc is available in the container via env vars | ||
ENV DOCKER_PROTOC_LOCATION=/protoc | ||
ENV DOCKER_PROTOC_VERSION="${PROTOC_VERSION}" | ||
|
||
# install grpc | ||
WORKDIR /grpc | ||
RUN source /src/utils/utilities.sh \ |
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.
source /src/utils/utilities.sh
is not needed anymore. We should probably split
RUN source /src/utils/utilities.sh \
&& download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"
to
RUN source /src/utils/utilities.sh
RUN download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"
Then here we can just do
RUN download_grpc_plugin "${GRPC_VERSION}" "${OS_ARCHITECTURE}"
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.
This is something I considered initially when working on the Dockerfile. The effects of source
are contained in the RUN
statement only, because it is translated into /bin/sh -c "source script.sh"
(see SO)
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.
Cool, that's good to know!
@@ -19,7 +19,9 @@ SHELL [ "/bin/bash", "-c" ] | |||
|
|||
ARG OWLBOT_CLI_COMMITTISH=ac84fa5c423a0069bbce3d2d869c9730c8fdf550 | |||
ARG PROTOC_VERSION=25.3 | |||
ARG GRPC_VERSION=1.62.2 |
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.
Can we configure this with renovate bot, and group it with other grpc update? e.g. There is an existing PR that updates gRPC to the latest.
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.
Done
sdk-platform-java/renovate.json
Lines 55 to 67 in 9795e23
{ | |
"customType": "regex", | |
"fileMatch": [ | |
"^gax-java/dependencies\\.properties$", | |
"^\\.cloudbuild/library_generation/library_generation\\.Dockerfile$" | |
], | |
"matchStrings": [ | |
"version\\.io_grpc=(?<currentValue>.+?)\\n", | |
"ARG GRPC_VERSION=[\"']?(?<currentValue>.+?)[\"']?\\s+" | |
], | |
"depNameTemplate": "io.grpc:grpc-core", | |
"datasourceTemplate": "maven" | |
}, |
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.
This version can be updated to 1.65.1 now since this PR is merged..
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.
Done
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.
We can still pass grpc_version
from generation_config.yaml
, I guess that would still override the baked in grpc version and the default grpc version from the pom?
Co-authored-by: Blake Li <blakeli@google.com>
@blakeli0 Yes, if sdk-platform-java/library_generation/generate_library.sh Lines 89 to 91 in 0a95554
sdk-platform-java/library_generation/generate_composed_library.py Lines 124 to 133 in 0a95554
sdk-platform-java/library_generation/utils/utilities.py Lines 31 to 39 in 0a95554
|
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
This PR bakes the gRPC plugin in the hermetic library generation image.
The approach is very similar to #2707
ARG
for the gRPC plugingenerate_library.sh
will now be able to avoid downloading the gRPC plugin if the computed plugin version is the same as DOCKER_GRPC_VERSION (all this viautilities.sh
).