search

owntracks / docker-recorder

Docker image for OwnTracks Recorder
151 stars 68 forks source link

Refactoring and cleanups #52

Closed stv0g closed 2 years ago

stv0g commented 2 years ago

I streamlined the Dockerfile which is now doing a multi-stage build which greatly simplifies it and improves cache utilization. I also fixed some Markdown issues in the README and made general code-style improvements to the shell scripts. I enabled IPv6 support in Mongoose.

stv0g commented 2 years ago

Just noticed that Travis is failing. I will fix this ASAP

jpmens commented 2 years ago

The build passed; I think it has something to do permissions.

jpmens commented 2 years ago

Merged. Thank you very much, Steffen!

jpmens commented 2 years ago

Image has shrunk a bit :-) 

REPOSITORY           TAG        IMAGE ID       CREATED         SIZE
owntracks/recorder   0.8.8-14   7556f8ad6c26   3 minutes ago   12.3MB
owntracks/recorder   latest     7556f8ad6c26   3 minutes ago   12.3MB
owntracks/recorder   0.8.8-13   1114758b7723   2 months ago    14.3MB
saesh commented 2 years ago

Hey!

Just wanted to let you know that this change together with this change now overrides OTR_TOPICS in the recorder.conf of owntracks-recorder itself.

This was somewhat unexpected as OTR_TOPICS is not supported as an environment variable in owntracks-recorder and the only way to restrict the topics on the recorder was using the recorder.conf file. Now docker-recorder governs this behavior by an environment variable OTR_TOPIC (note, without 's') making the use of OTR_TOPICS in the recorder.conf obsolete when using docker-recorder. The change was not easy to spot and I had to clean up quite a bit as my instances of owntracks-recorder where collecting data from all topics all of a sudden.

jpmens commented 2 years ago

@saesh thanks for reporting this, and I am very sorry it caused you additional work.

To be honest, I thought the idea was good because it would at least temporarily solve the issue of ot-recorder not yet supporting $OTR_TOPICS, but I didn't think it through when accepting this pull.

Right now I'm not sure whether we should retract this feature, as we simply don't know how many people will be inconvenienced either way...

I'm open to suggestions.

saesh commented 2 years ago

No worries @jpmens.

I think for newcomers to the project using docker-recorder it might be unclear how to achieve certain things as some configuration is done here (restricting topics), and some are done via environment variables in owntracks-recorder and yet some other are only supported using the recorder.conf file.

I would suggest to make most of the important configuration available through environment variables in owntracks-recorder. docker-recorder should be responsible for one thing only: making owntracks-recorder available through Docker and delegating any set of environment variables to owntracks-recorder (better yet without any knowledge what environment variables owntracks-recorder supports). That way it does not matter if you use owntracks-recorder by itself or through docker-recorder; the environment is the same (same env variables) only the underlying runtime is different. Then people can reference the official owntracks-recorder docs and use docker-recorder transparently.