kutzilla / docker-hetzner-ddns

A Docker image that allows you to use Hetzner DNS as a DynDNS Provider
MIT License
24 stars 8 forks source link

enhancement: added support for multiple subdomains #29

Open mteiting opened 1 year ago

mteiting commented 1 year ago

This change should resolve #22 without introducing breaking changes for existing installations.

Multiple subdomains can be added now by adding new environment named variables. Example:

RECORD_NAME_<NAME>=test
RECORD_NAME_<NAME>_TTL=600

RECORD_NAME_TEST1=test1
RECORD_NAME_TEST1_TTL=1440

RECORD_NAME_TEST2=test2
RECORD_NAME_TEST2_TTL=300

<NAME> does not need to match with the subdomain this is just an example.

Changes:

Remarks

kutzilla commented 1 year ago

Thank you for the time to implement this long awaited feature. I got some improvements regarding the functionality determining a single- or multi-domain case. You're reading the environment variables by the prefix "RECORD_NAME" and if there are multiple env's starting with the prefix, you set the variable useDefaultConfig to false. In my opinion this is kind of problematic, because if you got env's with the same prefix, but maybe for different purposes, this could lead to an issue, because you cannot set this to multiple-subdomain-mode. In those cases an additional variable to set it in multiple-subdomain-mode could be very helpful. Therefore the determination of the mode is not dependent on the presence of certain variables. Something like MULTIPLE_DOMAIN_MODE=true would be a good name.

What do you think?

mteiting commented 1 year ago

The problem with this approach is, that recordConf.mode is only set after flags.Read(). So this would never be true during setup.

You would have to manually parse for MULTI_DOMAIN_MODE during setup.

While I understand the problem with the environment variable prefixing, I don't see variable shadowing being the problem here. You need to make a decision between going single or multi-domain. The advantage you'd have with MULTI_DOMAIN_MODE that it makes your choice clear when you opt either approach.

kutzilla commented 1 year ago

Valid point, I agree with you. This approach won't be helping, if the variable MULTI_DOMAIN_MODE is not read yet.