feature: add tzdata and tini packages

TheCatLady / docker-webhook

A lightweight, minimal webhook container ⚓

https://hub.docker.com/r/thecatlady/webhook

MIT License

96 stars 21 forks source link

feature: add tzdata and tini packages #1

Closed onedr0p closed 3 years ago

onedr0p commented 3 years ago

Hello, thanks for the image!

I have a few changes I would like to add.

Allow setting a TZ environment variable for configuring the timezone via the tzdata package. This is helpful for seeing logs in your correct timezone.
Use tini as the entrypoint
Use an entrypoint.sh for running webhook with support for WEBHOOK_DEBUG and WEBHOOK_HOTRELOAD
No need to remove build deps in first stage, because you are using multi-stage build. Everything in the first stage gets tossed anyways.
Updated README.md with new environment variables

Let me know what you think, I am not the best with grammar so I probably messed up something in the README :)

TheCatLady commented 3 years ago

Hey @onedr0p, thanks so much for the PR! I'll take a look at it later and get back to you :) Are you setting the TZ environment variable in your docker run command or Compose YAML? I don't think tzdata is necessary here, but I'll take a closer look tonight.

onedr0p commented 3 years ago

Hey! Last I knew TZ only worked when the tzdata package is installed because alpine base images do not contain the timezone info tzdata provides.

There is an issue I cannot pinpoint in this, I cannot get -hooks=hook-1.json -hooks=hook-2.json or even just -hooks=hook-1.json to be read correctly. It always defaults to hooks.json :(

Not sure when I am doing wrong, maybe just because the hooks don't exist in my testing.

TheCatLady commented 3 years ago

@onedr0p Ah you're right, it does seem like tzdata was removed in Alpine v3+.

I am okay with adding tini and tzdata, but I reverted your changes that modified the default config. Let's not change that, since it'd break existing setups (including mine 😜).

The entrypoint.sh file is also unnecessary, so I removed that as well.

onedr0p commented 3 years ago

Sounds good, were you able to test whether or not the -hooks arg was working or not?

TheCatLady commented 3 years ago

@onedr0p Ah you're right, it does seem like tzdata was removed in Alpine v3+.

I am okay with adding tini and tzdata, but I reverted your changes that modified the default config. Let's not change that, since it'd break existing setups (including mine 😜).

The entrypoint.sh file is also unnecessary, so I removed that as well.

TheCatLady commented 3 years ago

@onedr0p

Sounds good, were you able to test whether or not the -hooks arg was working or not?

I did not verify this, but since the arguments just get passed to webhook, as long as the container has access to those files I don't see why it wouldn't work.