search

getodk / aggregate

ODK Aggregate is a Java server that stores, analyzes, and presents survey data collected using ODK Collect. Contribute and make the world a better place! ✨🗄✨
https://docs.opendatakit.org/aggregate-intro/
Other
74 stars 228 forks source link

Docker config improvements #439

Closed brettneese closed 5 years ago

brettneese commented 5 years ago

I'm working on deploying an Aggregate 2.0 instance to Kubernetes in the cleanest possible way, and documenting my process here.

This PR enables the ability to run the Gradle build commands for Docker without manually copying the *.example config files. I noticed that docker.gradle automatically overrides any settings in those files, anyway in the dockerPrepare build script so it made sense to have the build script just rename those files. Now you can build without manual work.

The only hacky thing I had to do was check the build commands sent to Gradle in the checkConf task against whether this was a build for Docker, which may be less than ideal, but seemed better than removing that check altogether. Otherwise if those files don't exist, Gradle throws an exception.

I'm happy to entertain alternative approaches, which is why I'm marking this WIP. I'm not very familiar with Gradle, so I'd love input there.

Part of the reason this matters is that the future I'm going to submit a PR that allows mounting those config files directly (instead of/in addition to the really hacky env var replacement thing going on in the entrypoint.) In K8S, this would be done using a ConfigMap -- you'll be able to just include the values of security.properties as a key in your ConfigMap, and mount it -- and I might be able to do something similar with Docker Compose. This seems a lot more sane that a mess of env vars that get replaced in files in the entrypoint.

What has been done to verify that this works as intended?

./gradlew clean dockerBuild -xtest -PwarMode=complete

Why is this the best possible solution? Were any other approaches considered?

It may not be. In fact, it's quite hacky. I'd love to hear about other approaches.

Are there any risks to merging this code? If so, what are they?

It may break other build processes. I haven't fully tested them, but it shouldn't.

Do we need any specific form for testing your changes? If so, please attach one

Nope.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

Yes, it will require an update to the Docker docs. I will be happy to update this PR with the appropriate changes if others agree this is a good idea.

ggalmazor commented 5 years ago

Hi, @brettneese!

This PR is awesome, thanks!!! I think we can move forward with that if in the checkConf task, no problem.

If you throw in the docs changes, I think we can approve the current commits as they are.

I'm very much looking forward to those K8S ~instructions~ walkthrough too. This is exciting!

brettneese commented 5 years ago

@ggalmazor Excellent! I did make one small change, and that was to remove the warning. If the files get overridden, then there's no point in warning anyway that they're missing (if anything, we should warn people that they're being overriden! But I'm noting that in the docs.)

brettneese commented 5 years ago

And I updated the docs!

ggalmazor commented 5 years ago

Since you tagged this PR as WIP, I'll wait for your confirmation in order to merge this PR

brettneese commented 5 years ago

I just marked it as not WIP, it's good to go! 👍 @ggalmazor :shipit: