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

Remove grpc-teleterm make target #17536

Closed
ravicious opened this issue Oct 18, 2022 · 2 comments
Closed

Remove grpc-teleterm make target #17536

ravicious opened this issue Oct 18, 2022 · 2 comments
Assignees
Labels
grpc Issues related to gRPC refactoring teleport-connect Issues related to Teleport Connect.

Comments

@ravicious
Copy link
Member

ravicious commented Oct 18, 2022

The grpc-teleterm target was introduced because at the time grpc-tools didn't provide arm64 binaries for both macOS and Linux. Reportedly, v1.11.3 added arm64 binaries for macOS but it's still missing arm64 binaries for Linux. This is blocking us from removing the target because make grpc-teleterm builds protobufs within a Linux container.

Linux binaries are not available because GitHub Actions don't provide Linux arm64 build images.

Removing grpc-teleterm and merging it into just the grpc target will speed up protobuf generation when developing Connect since there will be no need to build all those dependencies from source. This will also align Connect's protobuf generation with the rest of Teleport. We already had a couple of instances of people not knowing about the grpc-teleterm target and breaking protobuf generation when introducing changes for the grpc target.

What needs to be done

Everything that is in the grpc-teleterm target needs to be pulled into grpc. grpc uses build.assets/Dockerfile while grpc-teleterm uses build.assets/Dockerfile-teleterm. This means that whatever Dockerfile-teleterm adds on top of the regular buildbox Dockerfile needs to be pulled into that Dockerfile.

The whole grpc_node_plugin_binary_compiled branch of Dockerfile-teleterm becomes unnecessary once the prebuilt arm64 binaries are available for Linux.

We should also look into removing Dockerfile-teleterm itself if possible. Originally it was introduced just for grpc-teleterm but nowadays it's also used by drone in push and tag pipelines. That's because both generating protobufs and building Connect require Node.js and yarn so Dockerfile-teleterm is used for both purposes.

If merging deps from Dockerfile-teleterm into regular Dockerfile is resonable, then we should just drop the special Dockerfile and build everything with the regular one.

@ravicious ravicious added refactoring blocked is blocked by another item - please include the blocker teleport-connect Issues related to Teleport Connect. grpc Issues related to gRPC labels Oct 18, 2022
@ravicious
Copy link
Member Author

ravicious commented Dec 22, 2022

Alternative ideas:

  1. Precompile the binary for arm64 Linux once and then copy it over to the container. Add a make target for precompiling the binary.
  2. Migrate over to protobuf-es.

@ravicious ravicious self-assigned this Jan 5, 2023
@ravicious ravicious removed the blocked is blocked by another item - please include the blocker label Jan 5, 2023
@ravicious
Copy link
Member Author

Done in #20032.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grpc Issues related to gRPC refactoring teleport-connect Issues related to Teleport Connect.
Projects
None yet
Development

No branches or pull requests

1 participant