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

enhancement: added support for multiple subdomains #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mteiting
Copy link

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:

  • Added support for multiple subdomains while maintaining support for single domains
  • Changed fmt.Print* to log.Print* for logs with timestamp
  • Changed ioutil.ReadAll to io.ReadAll since ioutil.ReadAll is deprecated

Remarks

  • Failed to add test for testing single domain since it seems the same instance and variables cannot be added twice via flags
  • Only works with environment variables

@kutzilla
Copy link
Owner

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?

@kutzilla kutzilla self-assigned this Oct 17, 2023
kutzilla added a commit that referenced this pull request Oct 17, 2023
@mteiting
Copy link
Author

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
Copy link
Owner

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

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.

Update Multiple Record Names
3 participants