Conversation
…xpand cli options
This is bits and pieces of what I wanted from `cibuild-gh-ost-replica-tests` and `localtests/test.sh`
shlomi-noach
left a comment
shlomi-noach
left a comment
There was a problem hiding this comment.
Thank you so much for working on this! I did leave a few (non trivial) requests for changes. TL;DR remove code duplication, use gh-ost-ci-env repo, remove assumptions about binary being up to date.
| RUN apt-get update \ | ||
| && DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| mysql-server-${MYSQL_VERSION_APT} \ |
There was a problem hiding this comment.
Why do we need mysql-server? We are using dbdeployer-based server, right?
| wget \ | ||
| && apt-get autoremove | ||
|
|
||
| RUN wget https://github.com/datacharmer/dbdeployer/releases/download/v${DBDEPLOYER_VERSION}/dbdeployer-${DBDEPLOYER_VERSION}.linux.tar.gz \ |
There was a problem hiding this comment.
Would you be willing to use https://github.com/github/gh-ost-ci-env for:
- getting
dbdeployer - getting the various MySQL server versions?
This way both this docker image and the migration tests CI job will use the exact same dbdeployer+mysql versions. Please see how https://github.com/github/gh-ost/blob/master/.github/workflows/replica-tests.yml pulls the binaries.
| fi | ||
|
|
||
| echo "Creating replication sandbox" | ||
| dbdeployer deploy replication $MYSQL_VERSION \ |
There was a problem hiding this comment.
This sets up one master, two replicas (the default for dbdeployer replication setup). Let's save time and memory and set up a single replica, instead. Again, see https://github.com/github/gh-ost/blob/master/script/cibuild-gh-ost-replica-tests:
| @@ -0,0 +1,219 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This looks like a lot of duplication from https://github.com/github/gh-ost/blob/master/script/cibuild-gh-ost-replica-tests
Can we instead just call https://github.com/github/gh-ost/blob/master/script/cibuild-gh-ost-replica-tests? Or refactor into a more generic script? There's otherwise too much duplication; the entire test logic is rewritten here; I'd like to see both Docker and the GitHub CI use the same code.
| packages_path="${2:-/tmp/gh-ost-release}" | ||
| docker_target="gh-ost-packaging" | ||
| docker build . -f Dockerfile.packaging -t "${docker_target}" && docker run --rm -it -v "${packages_path}:/tmp/pkg" "${docker_target}:latest" bash -c 'find /tmp/gh-ost-release/ -maxdepth 1 -type f | xargs cp -t /tmp/pkg' | ||
| docker build . -f Dockerfile.packaging -t "${docker_target}" && docker run --rm -it --mount type=bind,source="${packages_path}",target=/tmp/pkg "${docker_target}:latest" bash -c 'find /tmp/gh-ost-release/ -maxdepth 1 -type f | xargs cp -t /tmp/pkg' |
There was a problem hiding this comment.
could you please explain this change?
| # | ||
| # Wrapper for running integration tests in docker. | ||
|
|
||
| ./script/dock pkg $(pwd)/bin |
There was a problem hiding this comment.
This seems to assume that the gh-ost binary in $(pwd)/bin is up to date and built from the current branch/changes. This may not be the case.
|
I haven't forgotten about this! I should have time later this week. |
|
Closing because dbdeployer is deprecated (#1529) |
4 participants
Related issue: #857
Description
This PR adds support for running
localtestsvia docker.It adds:
Testing
Example output: https://gist.github.com/ajm188/c83312ef82f06d51ca1f182c22b45d28