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

Flatten/Merge VTTOP and VTROOT variables #5527

Merged
merged 6 commits into from
Dec 10, 2019

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Dec 7, 2019

Fixes #5378 + includes a workaround for #5533

The motivation for this patch is that it allows one to checkout the source code, and run make build and then make test with no configuration required.

VTROOT is unfortunately still required for runtime, since configuration files and webfiles need to be located. We can fix this in #5502

The Dockerfiles use the historical layout still, with a symlink from the new 'bin' location back to /vt/bin.

The Docker test runner has been modified so that it will work without waiting for new docker images to be published. After this PR merges and we push new images, there is code in docker/test/run.sh which can be removed.

Note: This also disables the prepared statement test. See: #5524

Backwards Compatibility

This PR is intended to be mostly backwards compatible, however users will need to update their environment variables to point $VTROOT to the correct location (what was VTTOP).

This is not a problem in build or test, since build.env always overwrites $VTROOT and sets it to $PWD. But it is a problem in the examples/ since there are scripts which will attempt to include $VTROOT/web/vtctld. My hope is that we can fix this in #5502 before release, so that $VTROOT is not a run-time requirement at all.

Documentation

The changes to the build guides will be documented in vitessio/website#372

Disable prepared_statement test
Add a check to examples to make sure etcd is not running

Signed-off-by: Morgan Tocker <[email protected]>
Copy link
Contributor

@dkhenry dkhenry left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@sougou sougou left a comment

Choose a reason for hiding this comment

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

Looking good. Waiting for @dkhenry to eyeball it.

@derekperkins
Copy link
Member

This doesn't appear to impact any of the docker/k8s images which are what the helm charts use, so I don't think that it should cause any issues.

@morgo morgo merged commit aeab98e into vitessio:master Dec 10, 2019
@morgo morgo deleted the morgo-remove-vttop2 branch December 10, 2019 14:30
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.

Remove vttop / simplify build structure
4 participants