-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add vtadmin binary to release and Docker images #9405
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
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.
Are you using the k8s image? The standard image is
vitess/lite
. You are already addingvtadmin
to themake install
target, that gets it into all thelite
images. This change should not be required.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 publish all the individual component images from the
k8s
image, so IMO this should stay. There is probably an additional step required to copy out the files needed for the web. This stepCOPY --from=base $VTROOT/web /vt/web/
is what copies them for the existing UI, so I'd expect to see something similar for vtadmin.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.
Once this gets merged and we come up with an appropriate Dockerfile, I'll add a new directory and vtadmin image repo on Docker Hub that approximates the existing vtctld. https://github.com/vitessio/vitess/blob/main/docker/k8s/vtctld/Dockerfile
Side note: there's an open issue for consolidating images, but for now we still need to support people using
lite
and the component images. #7433There 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.
@derekperkins
I have added
/web/vtadmin
to the lite image.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.
btw there is a difference between vtadmin and orchestrator web files: orchestrator stores js/css files in final form in the repo, however, for
vtadmin
we copy the whole frontend project that needs to be built bynpm run build
- only then we get the final js/css files.This is not a problem since in my project I use
vitess/lite
image as a basebuilder
and then copy binaries/web files to final images (vtadmin-web, vtorc, vtgate, vtadmin-server).Example of how I do it for vtorc:
but IMO we should include the final build (js/css etc) somehow.
I see three potential ways of fixing it:
web/vtadmin
todocker/lite
and it's up to the user ofdocker/lite
to build the frontend. That adds 10M to thedocker/lite
size. This is what is done in that PR for now - https://github.com/vitessio/vitess/pull/9405/files#diff-bdcbcdaa92c26cad86d024c8d1061d14c36e20f67c5c8e577c17e3bbb196c607R54npm run build
as a part ofdocker/lite
build and copybuild/
dir into finaldocker/lite
. That adds additional 13M to the size ofdocker/lite
(just checked)vtadmin-web
image:node
Docker image as a buildernpm install
nginx
or another webserver)cc @doeg ^
I will be happy to send a PR if we decide on the final form.
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.
@jakon89 wow, thanks so much for the thorough notes! I really really appreciate it.
Between the three, I don't have a strong opinion. I'm happy to go with whatever is most ergonomic for Vitess operators! 🤔 The third option is the closest to what we do at Slack, though that doesn't mean it's the best for everyone. @jakon89 @derekperkins do either you have a preference?
P.S. There is definitely work to be done to make the front-end build smaller; the TypeScript generated from the protobufs is particularly huge.
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.
#3 is my preference, that for now I would put in
/docker/k8s/vtadmin
. You mentionedvtadmin-web
, are you building separate images for the server and the web ui?It's probably also worth building into
lite
per #2, but I'll leave that decision to @deepthi.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.
If we follow the same pattern as we do for Golang binaries, we should do #2. I'm fine with that. Not sure if you want to do that as a separate PR, or include it in this one. If this PR is complete, it LGTM.
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.
@deepthi let's merge that, I will implement building vtadmin-ui in new PR. Thanks