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

No version-controlling in appredict-chaste-libs Dockerfile #4

Closed
flawmop opened this issue May 30, 2023 · 4 comments
Closed

No version-controlling in appredict-chaste-libs Dockerfile #4

flawmop opened this issue May 30, 2023 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@flawmop
Copy link
Member

flawmop commented May 30, 2023

There doesn't appear to be any version-controlling in https://github.com/CardiacModelling/appredict-docker/blob/master/appredict-chaste-libs/Dockerfile, e.g. FROM debian:buster-slim will pull in the latest (which appears to updating regularly). There is also a RUN apt-get update, which while good for security, I suspect that this will mean that there's no certainty from one build to the next that a specific version of libboost, xerces, sundials, etc., will be the same. Whilst a container uploaded to Docker Hub represents a unique construct, the Dockerfile in its current form is permitting a fair degree of variation in the build versioning of dependencies.

This is also a similar scenario for https://github.com/CardiacModelling/ap-nimbus-client/blob/master/docker/Dockerfile.

If this is an issue, it may be better to assign the image digest (I think, I'd need to check up on how to find out how to retrieve that value, e.g.
https://stackoverflow.com/questions/71391031/how-can-i-specify-the-digest-to-be-used-in-a-dockerfile-from-line) and maybe not to do the apt-get update.

@flawmop flawmop added the enhancement New feature or request label May 30, 2023
@flawmop flawmop self-assigned this May 30, 2023
@mirams
Copy link
Member

mirams commented May 30, 2023

I guess as long as we are working with an Ubuntu LTS that passes all ApPredict's tests (now running on Github actions: https://github.com/Chaste/ApPredict/actions) then it shouldn't be an issue, and might be desirable to have it up to date with ubuntu's latest patches. I suppose there could be some check we have a passing ApPredict test suite on that Ubuntu?

@flawmop
Copy link
Member Author

flawmop commented May 30, 2023

If you're ok with the fluidity then all's good and I can close this issue as "Close as not planned" (and can perhaps shoehorn a note in the docs somewhere to indicate we've considered this).

If the (soon to be released) ApPredict container building instructions are followed there's a some optional steps (e.g. recommendations to test the built containerised ApPredict at each step) and some unavoidable steps (i.e. when building appredict-with-emulators and it runs ApPredict to generate the lookup tables) which should give further assurance that it's a fully working ApPredict being released to the container world.

@mirams
Copy link
Member

mirams commented May 30, 2023

Yeah, I guess the building instructions should feature a step like "run all the tests and check they pass" (is it possible to have the container take a command to run all the tests and check they pass?)

@flawmop
Copy link
Member Author

flawmop commented May 30, 2023

We can do. I'll close this and create a new issue because there are no doubt some best practices available (e.g. https://docs.docker.com/language/java/run-tests/#multi-stage-dockerfile-for-testing) to investigate further.

@flawmop flawmop closed this as completed May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants