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

Expose browser-apikey via environment variable #9

Merged
merged 1 commit into from Nov 20, 2016
Merged

Expose browser-apikey via environment variable #9

merged 1 commit into from Nov 20, 2016

Conversation

suhlig
Copy link
Contributor

@suhlig suhlig commented Nov 19, 2016

The recorder README states that OTR_BROWSERAPIKEY can be set to override the Google maps browser API key. Passing it to the Docker container does not seem to work with the current version of supervisord. The following snippet should have worked, but it does not:

[program:recorder]
environment=ENV_OTR_BROWSERAPIKEY=%(ENV_OTR_BROWSERAPIKEY)s
command=/usr/sbin/ot-recorder --http-host 0.0.0.0 'owntracks/#'

It can be checked pretty quickly after starting the container with curl http://my-docker-host:8083/static/apikey.js, which is still having an empty api key. There is some more evidence on supervisord's env issues in this SO thread.

This PR works around the issue by adding --browser-apikey to the invocation of ot-recorder in supervisord.conf, reading the value from the OTR_BROWSERAPIKEY environment variable. This, btw, which proves that passing the environment value from docker -e to supervisord works in general.

A change to run.sh shows how to set this environment variable.

It is possible that this is a bug in owntracks/recorder, but I was unable to reproduce it there.

Add `--browser-apikey` to `supervisord.conf`, reading its value from the `OTR_BROWSERAPIKEY` environment variable.
@jpmens jpmens merged commit 26345f6 into owntracks:master Nov 20, 2016
@jpmens
Copy link
Member

jpmens commented Nov 20, 2016

Sounds sensible (and I don't think it's a Recorder issue).
Merged, thank you.

@jpmens
Copy link
Member

jpmens commented Nov 20, 2016

I've kicked a new build on Dockerhub, and would appreciate if you could test it: the build is now ready.

@juzam
Copy link
Collaborator

juzam commented Nov 23, 2016

TL;DR the image now works only if a Broweser Api Key is provided.

The image pushed on the hub works only if an OTR_BROWSERAPIKEY variable is supplied at runtime.

I did some tests and without a key i.e. with a simple docker run -it owntracks/recorderd and the container dies since supervisord can't expand the variable:

Error: Format string "/usr/sbin/ot-recorder --http-host 0.0.0.0 --browser-apikey %(ENV_OTR_BROWSERAPIKEY)s 'owntracks/#'" for 'command' contains names which cannot be expanded

Running the container without providing a key was possible pior to this and an empty apikey.js was served. I need to figure out if it's possible to feed a NULL variable via ENV in the Dockerfile in order to restore the previous behaviour if that's a desiderata, (i don't think is possible via Dockerfile since my tests are not ok so far)

Another way to get the image work as before could be installing a more recent supervisord (via pip, that isn't included in the current Image) since the distro provided package is too old and has this bug

@jpmens
Copy link
Member

jpmens commented Nov 23, 2016

Oh rats!

@juzam juzam self-assigned this Nov 24, 2016
@juzam juzam mentioned this pull request Nov 25, 2016
juzam added a commit to juzam/recorderd that referenced this pull request Nov 26, 2016
juzam added a commit that referenced this pull request Nov 26, 2016
@juzam
Copy link
Collaborator

juzam commented Nov 26, 2016

@suhlig no need to update supervisord, the behaviour you have experienced was due to the ot-recorder debian package version 0.6.8. as of yesterday 0.6.9 is available in the recorder debian repo and is included in the Image. Now passing the variable OTR_BROWSERAPIKEY via docker -e works as expected. no further configuration needed in supervisord.

@jpmens
Copy link
Member

jpmens commented Nov 26, 2016

Thank you @juzam for taking care of this and apologies that I hadn't released the updated package earlier.

@juzam
Copy link
Collaborator

juzam commented Nov 26, 2016

@jpmens there's no need to apologize. this has been a crash course in git/github/docker debugging. I'm always happy to be helpful.

juzam added a commit that referenced this pull request Dec 28, 2016
* rollback upstream PR #9

* testing ot-recorder.defaults in volume

Signed-off-by: Giovanni Angoli <juzam76@gmail.com>

* moved topics configuration from supervisord.conf to ot-recorder.default so it can be overridden
@suhlig suhlig deleted the google-api-key branch June 18, 2017 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants