-
Notifications
You must be signed in to change notification settings - Fork 225
Added prerequisites to docker-install #1777
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
Closed
kirtanchandak
wants to merge
3
commits into
vitessio:prod
from
kirtanchandak:local-docker-install/kirtan
Closed
Changes from all commits
Commits
Show all changes
3 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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.
Uh oh!
There was an error while loading. Please reload this page.
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.
Applies to all versions: I am unsure why we require Golang since we do everything on Docker.
Is it because eventually the guide links to the
Move Tablesstep and in that step we need to run somevtctldclientcommands? If so, we should add a step that builds that binary along with a prerequisite to havemysqlinstalled since we use it in the next step too.Uh oh!
There was an error while loading. Please reload this page.
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.
Actually I think it is a mistake to link the Docker install guide to the current
Move Tablesguide. In theMove Tablesguide we only support K8S and local installs. The local install steps won't work on Docker as we start/execute the binaries directly on the user's machine, not in Docker. For instancemysqlctl-up.shwhich is called by./201_customer_tablets.shdoes the following:In that case, I am still unsure why we require
Golangto be installed for the Docker guide.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.
make docker_localitself requires golangUh oh!
There was an error while loading. Please reload this page.
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 suppose we could remove this link. Or remove docker_local completely. Who is using it?
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.
Removing it has been my long running preference FWIW.
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.
hmm, actually not. So we need the PR author to tell us what the specific error was that required golang installation.
But the bigger point is what Matt said. Should we delete this guide completely? We've debated this in the past without coming to a good conclusion.
Maybe what we should do is conduct a survey in slack and if no one is using it, we delete the code and docs.
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.
It is debatable but I don't think we should require people to install Golang if they use the Docker install guide, it defeats the purpose of using Docker and contradicts the first sentence of the guide:
If we decide to keep this guide and these commands on the Makefile, I think we should tweak
build_docker_imageto first check if Go is installed before calling it, and if it is not installed: build the docker images as usual (without the arch/proc information). The guide also saysThis guide will only work on x86_64/amd64 based machines.which, if I understand correctly, is not fully true if you have Golang installed since we use the--platformflag to build the Docker images.I think we should put more emphasis on inviting users to use K8S instead of Docker, the install steps of the operator are easier to follow than they were a few years ago. But I also understand that some people want to have a small vitess setup to test things around in which case this guide is useful, but should not necessarily be linked to
Move Tables.Uh oh!
There was an error while loading. Please reload this page.
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 am guessing the error the author saw is that the following if statement from
build_docker_imageis broken:If go is not installed, the condition will always be true, leading to wrong arguments when calling
docker buildx build --platform "$$(go env GOOS)/$$(go env GOARCH)" -f ${1} -t ${2} --build-arg bootstrap_version=${BOOTSTRAP_VERSION} .; \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.
Easily testable with this
makecommand that runsgox(does not exist) instead ofgo:Which returns: