Remove grpc-teleterm Make target and Dockerfile-teleterm #20032
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ravicious
force-pushed
the
ravicious/remove-grpc-teleterm
branch
2 times, most recently
from
b574001 to
c5cceba
Compare
ravicious
force-pushed
the
ravicious/remove-grpc-teleterm
branch
from
c5cceba to
4e86f38
Compare
ravicious
commented
Jan 11, 2023
| @@ -7,7 +7,11 @@ | |||
| # teleport built on any newer Ubuntu version will not run on Centos 7 because | |||
| # of this. | |||
|
|
|||
| ARG RUST_VERSION | |||
There was a problem hiding this comment.
RUST_VERSION was defined twice in Dockerfile.
Comment on lines
+30
to
+36
| # Generate lib/teleterm & JS protos. | ||
| # TODO(ravicious): Refactor generating JS protos to follow the approach from above, that is have a | ||
| # separate call to generate Go protos and another for JS protos instead of having | ||
| # teleterm-specific buf.gen.yaml files. | ||
| # https://github.com/gravitational/teleport/pull/19774#discussion_r1061524458 | ||
| buf generate --template=lib/prehog/buf-teleterm.gen.yaml lib/prehog/proto | ||
| buf generate --template=lib/teleterm/buf.gen.yaml lib/teleterm/api/proto |
There was a problem hiding this comment.
Originally I wanted to do this in this PR. But the list of changes is already big enough so I'll handle that in a separate PR (which I'm working on at the moment).
ravicious
commented
Jan 11, 2023
|
|
||
| # A "conditional" FROM like this breaks up the number of steps in the progress bar of the given | ||
| # image, so let's use it at the end. This way the progress bar for buildbox stays mostly untouched. | ||
| FROM grpc_node_plugin_binary_${GRPC_NODE_PLUGIN_BINARY_TYPE} as buildbox |
There was a problem hiding this comment.
This conditional FROM is based on this SO answer: https://stackoverflow.com/a/60820156/742872
ravicious
force-pushed
the
ravicious/remove-grpc-teleterm
branch
from
4e86f38 to
2d6ad31
Compare
mdwn
approved these changes
Jan 17, 2023
It was used to format protos but we use Buf for that since v10.
This commit basically takes grpc_node_plugin compilation from Dockerfile-teleterm and moves it to Dockerfile.
After moving grpc_node_plugin compilation to Dockerfile, the only remaining thing that Dockerfile-teleterm does is installing rpm so that we can make an RPM package for Connect during tag builds. Installing this package can be simply moved to Dockerfile.
It looks like they're a result of someone changing protos in lib/prehog without running `make grpc-teleterm` separately. Which is why we're getting rid of grpc-teleterm as a separate Make target in the first place. ;)
ravicious
force-pushed
the
ravicious/remove-grpc-teleterm
branch
from
2d6ad31 to
d983f38
Compare
|
Rebased on master with very minimal changes. During the webapps merge, #20361 was merged as well which also added Node.js to build.assets/Dockerfile so I removed that from my commit. I'm about to start a new tag build to verify that everything still works. https://drone.platform.teleport.sh/gravitational/teleport/19554 Edit: It went through! |
|
@fspmarshall Ping. |
fspmarshall
approved these changes
Jan 25, 2023
|
@ravicious See the table below for backport results.
|
ravicious
added a commit
that referenced
this pull request
Feb 1, 2023
* Remove CLANG_FORMAT from Makefiles It was used to format protos but we use Buf for that since v10. * Move installing grpc_node_plugin into Dockerfile This commit basically takes grpc_node_plugin compilation from Dockerfile-teleterm and moves it to Dockerfile. * Replace Dockerfile-teleterm with Dockerfile After moving grpc_node_plugin compilation to Dockerfile, the only remaining thing that Dockerfile-teleterm does is installing rpm so that we can make an RPM package for Connect during tag builds. Installing this package can be simply moved to Dockerfile. * Remove grpc-teleterm Make target in favor of grpc
ravicious
added a commit
that referenced
this pull request
Feb 6, 2023
`make protos-up-to-date` and thus `make grpc` is now run as a part of CI. Thus I cannot just change the buildbox Dockerfile when backporting #20032 as it will use the old buildbox image which won't be able to compile JS protos. I extracted relevant Dockerfile changes from the backport PR (#21064). Once this commit lands in v11, I should be able to make CI pass for the backport PR.
ravicious
added a commit
that referenced
this pull request
Feb 6, 2023
* Remove CLANG_FORMAT from Makefiles It was used to format protos but we use Buf for that since v10. * Move installing grpc_node_plugin into Dockerfile This commit basically takes grpc_node_plugin compilation from Dockerfile-teleterm and moves it to Dockerfile. * Replace Dockerfile-teleterm with Dockerfile After moving grpc_node_plugin compilation to Dockerfile, the only remaining thing that Dockerfile-teleterm does is installing rpm so that we can make an RPM package for Connect during tag builds. Installing this package can be simply moved to Dockerfile. * Remove grpc-teleterm Make target in favor of grpc
ravicious
added a commit
that referenced
this pull request
Feb 6, 2023
`make protos-up-to-date` and thus `make grpc` is now run as a part of CI. Thus I cannot just change the buildbox Dockerfile when backporting #20032 as it will use the old buildbox image which won't be able to compile JS protos. I extracted relevant Dockerfile changes from the backport branch. Once this commit lands in v10, I should be able to make CI pass for the backport PR.
This was referenced Feb 6, 2023
ravicious
added a commit
that referenced
this pull request
Feb 7, 2023
`make protos-up-to-date` and thus `make grpc` is now run as a part of CI. Thus I cannot just change the buildbox Dockerfile when backporting #20032 as it will use the old buildbox image which won't be able to compile JS protos. I extracted relevant Dockerfile changes from the backport PR (#21064). Once this commit lands in v11, I should be able to make CI pass for the backport PR.
ravicious
added a commit
that referenced
this pull request
Feb 7, 2023
…1064) * Remove CLANG_FORMAT from Makefiles It was used to format protos but we use Buf for that since v10. * Move installing grpc_node_plugin into Dockerfile This commit basically takes grpc_node_plugin compilation from Dockerfile-teleterm and moves it to Dockerfile. * Replace Dockerfile-teleterm with Dockerfile After moving grpc_node_plugin compilation to Dockerfile, the only remaining thing that Dockerfile-teleterm does is installing rpm so that we can make an RPM package for Connect during tag builds. Installing this package can be simply moved to Dockerfile. * Remove grpc-teleterm Make target in favor of grpc
ravicious
added a commit
that referenced
this pull request
Feb 7, 2023
`make protos-up-to-date` and thus `make grpc` is now run as a part of CI. Thus I cannot just change the buildbox Dockerfile when backporting #20032 as it will use the old buildbox image which won't be able to compile JS protos. I extracted relevant Dockerfile changes from the backport branch. Once this commit lands in v10, I should be able to make CI pass for the backport PR.
ravicious
added a commit
that referenced
this pull request
Feb 8, 2023
* Remove grpc-teleterm Make target and Dockerfile-teleterm (#20032) * Remove CLANG_FORMAT from Makefiles It was used to format protos but we use Buf for that since v10. * Move installing grpc_node_plugin into Dockerfile This commit basically takes grpc_node_plugin compilation from Dockerfile-teleterm and moves it to Dockerfile. * Replace Dockerfile-teleterm with Dockerfile After moving grpc_node_plugin compilation to Dockerfile, the only remaining thing that Dockerfile-teleterm does is installing rpm so that we can make an RPM package for Connect during tag builds. Installing this package can be simply moved to Dockerfile. * Remove grpc-teleterm Make target in favor of grpc * Install protoc-gen-go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
grpc-teleterm and Dockerfile-teleterm were originally added because generating JS protobufs on arm64 involved compiling grpc_node_plugin from source. It's a binary that needs a lot of deps and compiling it takes a few minutes even on new MBPs.
In order to not have every dev on arm64 pay this cost just so that the Connect team can generate JS protobufs, we decided to create a separate Dockerfile and use a separate Make target.
It's been a year and Connect became even more reliant on code outside of
lib/teleterm. We now want to share protos withlib/prehog(#19774) and Dockerfile-teleterm started being used to create Linux packages for Connect. Having a separate target doesn't make sense anymore and thanks to what Alan did with libfido2 in the Dockerfile, I found a way to build grpc_node_plugin once and cache the result.This allows us to remove grpc-teleterm in favor of adding lib/teleterm proto generation to
make grpc.Since Dockerfile-teleterm was used during tag builds, I made 12.0.0-dev.ravicious.2 that used changes from this PR and it passed successfully. That was before a rebase on master which didn't include some minor changes to JS protos, so I'm running another tag build, 12.0.0-dev.ravicious.3, to verify again that everything's okay.