-
Notifications
You must be signed in to change notification settings - Fork 36
Provide Dockerfile for initial MNTN-related testing. #171
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
base: tls
Are you sure you want to change the base?
Conversation
This Dockerfile builds a Debian Trixie base image with the latest Rust installed. It then executes a Bash script named "startup-sequence", whose job it is to run the `cargo test` command to invoke tests. Note that some environment variables are required when running the resulting Docker image (see Rust client docs). As more property-based testing facilities are provided, this Dockerfile and startup-sequence may be amended to take additional configuration via environment variables.
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.
LGTM, this is fine since we need to get the rests running ASAP. Just something to note in the future:
- We should add a README explaining what the Dockerfile is used for
- Also you can
chmod u+xthe startup script and commit those permissions in the repo - There's an official image for Rust: https://hub.docker.com/_/rust
- To test the Dockerfile, you can use it to run the tests in CI/CD
v2 is an older branch than tls; tls is where all mainline development is being staged for MNTN.
tools/docker/Dockerfile
Outdated
|
|
||
| COPY startup-sequence /app/startup-sequence | ||
| RUN chmod +x /app/startup-sequence | ||
| RUN echo 'source $HOME/.cargo/env' >> $HOME/.bashrc |
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.
This is already sourced in the script, why add it here?
tools/docker/startup-sequence
Outdated
|
|
||
| git clone https://github.com/aerospike/aerospike-client-rust.git | ||
| cd aerospike-client-rust | ||
| git checkout tls |
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 believe the name of the branch should be an ENV variable.
tools/docker/startup-sequence
Outdated
| cd aerospike-client-rust | ||
| git checkout tls | ||
| cargo clean | ||
| cargo fix --lib -p aerospike-core |
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.
Should be removed. Fixes should be applied to the source, otherwise tests will run on different source code.
tools/docker/startup-sequence
Outdated
| git checkout tls | ||
| cargo clean | ||
| cargo fix --lib -p aerospike-core | ||
| cargo test |
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.
Where do the ENV variables come from? IIRC they need to be explicitly passed here, no?
|
Also noticed that Rust client code seems to be building when running the container. Shouldn't this already be built in the image? |
This is what I was talking about yesterday regarding the |
This Dockerfile builds a Debian Trixie base image with the latest Rust installed. It then executes a Bash script named "startup-sequence", whose job it is to run the
cargo testcommand to invoke tests. Note that some environment variables are required when running the resulting Docker image (see Rust client docs).As more property-based testing facilities are provided, this Dockerfile and startup-sequence may be amended to take additional configuration via environment variables.