Skip to content

Added prerequisites to docker-install#1777

Closed
kirtanchandak wants to merge 3 commits intovitessio:prodfrom
kirtanchandak:local-docker-install/kirtan
Closed

Added prerequisites to docker-install#1777
kirtanchandak wants to merge 3 commits intovitessio:prodfrom
kirtanchandak:local-docker-install/kirtan

Conversation

@kirtanchandak
Copy link
Collaborator

  • added prerequisite to install go

image

Signed-off-by: kirtanchandak <themarketingkirtan@gmail.com>
Signed-off-by: kirtanchandak <themarketingkirtan@gmail.com>
@netlify
Copy link

netlify bot commented Jun 28, 2024

Deploy Preview for vitess ready!

Name Link
🔨 Latest commit 30da938
🔍 Latest deploy log https://app.netlify.com/sites/vitess/deploys/667fc165f009040008bdd650
😎 Deploy Preview https://deploy-preview-1777--vitess.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same changes should be made in all active versions. v18 -> v21.

## Prerequisite

Before we get started, let’s get a few pre-requisites out of the way:
1. Install [Golang](https://go.dev/doc/install) locally.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use bullet instead of numbering.


## Prerequisite

Before we get started, let’s get a few pre-requisites out of the way:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really need this line at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made the required changes

Signed-off-by: kirtanchandak <themarketingkirtan@gmail.com>
Comment on lines +15 to +18
## Prerequisite

- Install [Golang](https://go.dev/doc/install) locally.

Copy link
Member

@frouioui frouioui Jul 2, 2024

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 Tables step and in that step we need to run some vtctldclient commands? If so, we should add a step that builds that binary along with a prerequisite to have mysql installed since we use it in the next step too.

Copy link
Member

@frouioui frouioui Jul 2, 2024

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 Tables guide. In the Move Tables guide 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 instance mysqlctl-up.sh which is called by ./201_customer_tablets.sh does the following:

mysqlctl \
 --log_dir $VTDATAROOT/tmp \
 --tablet_uid $uid \
 --mysql_port $mysql_port \
 $action

In that case, I am still unsure why we require Golang to be installed for the Docker guide.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make docker_local itself requires golang

Copy link
Collaborator

@deepthi deepthi Jul 2, 2024

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 Tables guide.

I suppose we could remove this link. Or remove docker_local completely. Who is using it?

Copy link
Member

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make docker_local itself requires golang

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.

Copy link
Member

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:

but without having to install software on one's host other than Docker

If we decide to keep this guide and these commands on the Makefile, I think we should tweak build_docker_image to 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 says This 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 --platform flag 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.

Copy link
Member

@frouioui frouioui Jul 2, 2024

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_image is broken:

elif [ $$(go env GOOS) != $$(go env GOHOSTOS) ] || [ $$(go env GOARCH) != $$(go env GOHOSTARCH) ]; then \`

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} .; \

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easily testable with this make command that runs gox (does not exist) instead of go:

toto:
	if [ $$(gox env GOOS) != $$(gox env GOHOSTOS) ] || [ $$(gox env GOARCH) != $$(gox env GOHOSTARCH) ]; then \
		echo "Building docker using buildx --platform=$$(gox env GOOS)/$$(gox env GOARCH)"; \
	else \
		echo "Building docker using straight docker build"; \
	fi

Which returns:

/bin/bash: gox: command not found
/bin/bash: gox: command not found
/bin/bash: gox: command not found
/bin/bash: gox: command not found
Building docker using buildx --platform=/

@deepthi
Copy link
Collaborator

deepthi commented Jul 2, 2024

@kirtanchandak after discussion with @frouioui this is what we concluded

  • we will close this PR
  • can you do a new PR that removes the reference to MoveTables at the end? Just removing these two lines
Next Steps [#](https://vitess.io/docs/21.0/get-started/local-docker/#next-steps)

You can now proceed with [MoveTables](https://vitess.io/docs/21.0/user-guides/migration/move-tables).
  • go over to the main vitessio/vitess repo and make a PR to fix the go error from make docker_local
  • @frouioui @mattlord and I will discuss with other maintainers whether to delete this guide completely

@deepthi deepthi closed this Jul 2, 2024
@frouioui
Copy link
Member

frouioui commented Jul 2, 2024

Correct, @kirtanchandak I can give you some pointers on how to fix the error. In the Makefile we have a function called build_docker_image this function builds the Docker images differently depending on the arch/proc we want to use. Currently, if the image we want to build contains arch/proc information in its name, we use this information, this case should stay as it is. Then, we have an elif to check some Go variables, we should however first check that Go is installed and only do the rest of the elif if it is installed. Finally, in the else statement, we should keep doing what we do which is to build the Docker image without any arch/proc information. You can easily check that Go is installed by doing go version and checking the exit code.

@deepthi
Copy link
Collaborator

deepthi commented Jul 2, 2024

@kirtanchandak please wait a day or two until the team makes a decision on whether to delete this guide. If that's what we decide your PR can delete the guide instead of modifying it.
That will also mean that we remove the docker_local target from the Makefile instead of fixing it to work when go is not installed.

@deepthi
Copy link
Collaborator

deepthi commented Jul 3, 2024

@kirtanchandak please go ahead and do 2 PRs

  • website PR to remove the "Local docker" getting started guide only from the latest aka v21.0 docs
  • vitessio/vitess PR to remove the docker_local target from the Makefile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants