Docker/Development Containers support #1406
Conversation
There was a problem hiding this comment.
Thank you for this PR. A one-click method for spinning up a Docker container with Synthea would be cool.
That said, this PR contains a lot of dependencies and files that are unrelated to that goal. Please remove the Makefile, editor configuration file and dependency on hermit. Additional detail is included in comments.
| @@ -0,0 +1,29 @@ | |||
| # EditorConfig is awesome: https://EditorConfig.org | |||
There was a problem hiding this comment.
Personal source code editor configuration files should not be a part of this PR.
There was a problem hiding this comment.
I disagree. .editorconfig is intended to ensure any .editorconfig compliant editor (most modern editors/IDEs) comply to the standards used in the repo. otherwise you will spurious indent changes from contributors.
There was a problem hiding this comment.
Synthea uses checkstyle for this purpose.
|
|
||
| COPY --chown=${USER}:${USER} ./ ./synthea | ||
|
|
||
| # install hermit and binary deps |
There was a problem hiding this comment.
It is unclear to me why you would want to use hermit in a Docker environment. I understand that hermit allows you to specify the exact version of tools that you want to use for development, such as the JDK. That said, you can do the same thing with plain Docker.
There was a problem hiding this comment.
you can, but we've used hermit in Dockerfiles for years with great success. hermit provides a simple way to ensure local environment binary tools exactly match docker container tools. and it simplifies installation (no need to look up the exact installation URL and procedure for each tool
| @@ -0,0 +1,20 @@ | |||
| .PHONY: build install visualize | |||
There was a problem hiding this comment.
Synthea uses gradle for its build system. Adding a Makefile replicates gradle functionality and will likely lead to confusion for users.
The project does not incorporate any tool management systems such as hermit, SDKMAN or jabba. Doing so would likely cause issues for users who may not be able to install these types of tools.
There was a problem hiding this comment.
the Makefile is a minor convenience mostly for installation. I could replace with an install.sh script
| @@ -39,7 +39,7 @@ | |||
| /** | |||
| * This is a rudimentary check of the CCDA Export. It uses XPath expressions to ensure that the | |||
| * mandatory sections for the Continuity of Care Document (CCD) (V3) are present as specified in | |||
| * HL7 Implementation Guide for CDA® Release 2: Consolidated CDA Templates for Clinical Notes | |||
| * HL7 Implementation Guide for CDA(R) Release 2: Consolidated CDA Templates for Clinical Notes | |||
There was a problem hiding this comment.
This change is harmless, but also not in any way related to the purpose of the PR.
There was a problem hiding this comment.
this was causing a build failure due to this special character. some sort of characterset issue. not sure how to fix so removal was the only option
| @@ -55,7 +55,7 @@ public void testCvdModelPatient2() { | |||
| Risk Factor Value Points | |||
| Age 53 8 | |||
| Total cholesterol 161 1 | |||
| HDL 55 −1 | |||
| HDL 55 -1 | |||
There was a problem hiding this comment.
Same comment as before, it's unclear what this has to do with the PR.
There was a problem hiding this comment.
this was causing a build failure due to this special character. some sort of characterset issue. not sure how to fix so removal was the only option
There was a problem hiding this comment.
Hi @rolandknight, will you be fixing this issue and other suggestions made by @eedrummer?
It will be great if your pull request could be merged in the main branch.
I will be happy to be the first user of your contribution.
Thanks
Added support for Development Containers. If you have docker and VSCode installed, you can open a fully functional environment with one click (click the DevContainers link at the top of the README.md file)