-
Notifications
You must be signed in to change notification settings - Fork 800
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
Upgrades python version for cqlsh #4709
Conversation
Dockerfile
Outdated
|
||
COPY docker/start.sh /start.sh | ||
|
||
# test installed applications | ||
RUN cqlsh --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.
after testing this wouldn't have actually caught the cqlsh issue, which fails at runtime only when executing, but it would have caught other issues, such as failure to correctly install to path.
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.
maybe pip3 install cqlsh && cqlsh --version
so we don't make another layer?
but probably not a bad idea, since the install is far from a guarantee with python.
I suppose it's somewhat odd that there's no support for testing builds in dockerfiles 🤔
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 think normally you treat them as build artifacts and subject them to e2e testing as you would any other deployable
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.
yeah, though in-dockerfile would let you test things as you build rather than only at the end. e.g. for from scratch
things, you could test in a build-and-debug-friendly env before you copy the binaries to a minimal image.
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.
great, thanks! simple fixes are always nice :)
and it's good to leave py2. it's definitely time to do so.
Pull Request Test Coverage Report for Build 2e7767c2-235d-4c24-a4ff-f23a937c339e
💛 - Coveralls |
What changed?
Why?
cqlsh is broken
How did you test it?
Potential risks
Release notes
Documentation Changes