Replace grpc tools npm package with a linux container#9837
Replace grpc tools npm package with a linux container#9837tobias-jarvelov wants to merge 8 commits intomainfrom
Conversation
da12879 to
be3a8a5
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
be3a8a5 to
2f60f54
Compare
This container builds grpc-node from scratch in order to avoid being dependent on downloading and running grpc-tools' prebuilt binaries. See background for this motivation in DES-2826
The generated bindings should now be commited to the repo.
2f60f54 to
d66a9c2
Compare
d66a9c2 to
9b73560
Compare
faern
left a comment
There was a problem hiding this comment.
@faern made 6 comments.
Reviewable status: 0 of 17 files reviewed, 6 unresolved discussions (waiting on @tobias-jarvelov).
.github/workflows/desktop-grpc-bindings.yml line 2 at r1 (raw file):
--- name: Desktop check grpc bindings
Nit. Please capitalize as gRPC for easier reading IMO
.github/workflows/desktop-grpc-bindings.yml line 5 at r1 (raw file):
on: pull_request: paths:
We usually include the workflow itself here. So we run any workflow as it is changed, to be able to catch issues in the updated workflow.
.github/workflows/desktop-grpc-bindings.yml line 23 at r1 (raw file):
- name: Install dependencies working-directory: desktop shell: bash
Nit. shell does not need to be specified for jobs only running on Linux. This is only needed on Windows runs.
desktop/packages/management-interface/Dockerfile line 1 at r1 (raw file):
FROM debian@sha256:77f46c1cf862290e750e913defffb2828c889d291a93bdd10a7a0597720948fc
I'd love some comments/documentation at the top of this dockerfile that covers at least:
- What is this container for: Generating the TypeScript bindings for management interface proto bindings
- This exact debian image hash, which debian version is that and should it be kept in sync with any other Dockerfile?
desktop/packages/management-interface/Dockerfile line 10 at r1 (raw file):
ARG GRPC_NODE_REF=ccd29b27d28ce8937f8250f72e5e6027ed5af09a # === Setup build folder ===
Build of what? Maybe we should name this folder /grpc directly to make things clearer?
desktop/packages/management-interface/scripts/container-build-image.sh line 14 at r1 (raw file):
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" CONTAINER_DIR="$( cd "$SCRIPT_DIR/.." && pwd )" IMAGE_NAME="grpc-js-ts-bindings"
Maybe we want to name this clearer in the context of our project. By prefixing it with mullvadvpn-app- or similar, like the other containers?
If we are going to use the container in the github actions we must also publish the image to ghcr.io like the other build containers. And then we should probably tag it accordingly, like the other containers for building.
... So that the full image name becomes something like this?:
ghcr.io/mullvad/mullvadvpn-app-build-proto-ts-bindings
faern
left a comment
There was a problem hiding this comment.
@faern made 1 comment.
Reviewable status: 0 of 17 files reviewed, 7 unresolved discussions (waiting on @tobias-jarvelov).
.github/workflows/desktop-grpc-bindings.yml line 29 at r1 (raw file):
working-directory: desktop shell: bash run: npm run build -w management-interface
Can't we just run ./scripts/container-run-generate-bindings.sh directly here? Would that not allow us to skip the npm ci step above? Seems like we do a lot of setup here just to run stuff in a container.
faern
left a comment
There was a problem hiding this comment.
@faern made 1 comment.
Reviewable status: 0 of 17 files reviewed, 8 unresolved discussions (waiting on @tobias-jarvelov).
desktop/packages/management-interface/scripts/container/build-grpc-tools-binaries.sh line 1 at r1 (raw file):
#!/usr/bin/env bash
Can't we just put all of these commands directly in the Dockerfile and get rid of the script? It's not that many commands. And then the dockerfile becomes more self contained.
faern
left a comment
There was a problem hiding this comment.
@faern made 1 comment.
Reviewable status: 0 of 17 files reviewed, 9 unresolved discussions (waiting on @tobias-jarvelov).
desktop/packages/management-interface/scripts/container/build-grpc-tools-binaries.sh line 12 at r1 (raw file):
# 3. Copies node protoc files to the node_modules/.bin folder # # Please refer to the documentation for more information on how to use
Which documentation? Would be nice with a relative path to what file this refers to
faern
left a comment
There was a problem hiding this comment.
@faern made 1 comment.
Reviewable status: 0 of 17 files reviewed, 9 unresolved discussions (waiting on @tobias-jarvelov).
desktop/packages/management-interface/Dockerfile line 10 at r1 (raw file):
Previously, faern (Linus Färnstrand) wrote…
Build of what? Maybe we should name this folder
/grpcdirectly to make things clearer?
I see now that the input proto files and output TS files are also using /build. I think we should separate the gprc-tools/grpc-node install dir from the dir used at runtime to generate bindings. One is part of the software package bundled in the image, the other is storage to transfer stuff in and out of the container at runtime.
I propose using /gprc-tools or similar in here to build and install grpc-node into. And then using maybe /proto to mount proto files into and /output to generate TS to?
This PR replaces the
grpc-toolsnpm dependency with a container. The npm dependency downloaded pre-compiled binaries and this is something we want to avoid having in our build chain. The container uses thegrpc-noderepository and builds thegrpc-toolsbinaries from source. The gRPC generated gRPC bindings have been committed and must be updated when the proto file is changed.Other changes:
podmanas a build dependency.This change is