Skip to content

HOSTNAME and DOMAINNAME setting improvements#2175

Merged
polarathene merged 11 commits intodocker-mailserver:masterfrom
NorseGaud:better-hostname-check
Sep 11, 2021
Merged

HOSTNAME and DOMAINNAME setting improvements#2175
polarathene merged 11 commits intodocker-mailserver:masterfrom
NorseGaud:better-hostname-check

Conversation

@NorseGaud
Copy link
Copy Markdown
Member

@NorseGaud NorseGaud commented Sep 7, 2021

Description

Problem: I don't use a subdomain, so HOSTNAME in the code becomes just the TLD of my domain and causes several issues (primarily with _monitored_files_checksums). I think requiring OVERRIDE_HOSTNAME adds unnecessary complexity and isn't easy to figure out for new users without spending some time.

Solution: Centralize the collection of the HOSTNAME and DOMAINAME so that it's predictable and uniform across the various scripts (using the helper). Ensure it supports the various configurations users can have (both subdomain and without subdomain, override and no override).

Also, DOMAINNAME as an env is supported for overriding SRS_DOMAINNAME, but not any other logic. Now that it's all using the same logic, specifying DOMAINNAME should override anywhere it shows up and is used in the code.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change that does improve existing functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md or the documentation under docs/)
  • If necessary I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@NorseGaud NorseGaud self-assigned this Sep 7, 2021
@NorseGaud NorseGaud added kind/improvement Improve an existing feature, configuration file or the documentation priority/low labels Sep 7, 2021
@NorseGaud NorseGaud changed the title Better hostname check HOSTNAME and DOMAINNAME setting improvements Sep 8, 2021
@NorseGaud NorseGaud requested a review from a team September 8, 2021 19:55
@NorseGaud NorseGaud marked this pull request as ready for review September 8, 2021 19:57
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic-wise looks absolutely fine to me - LGTM 👍 Just two lines of formatting :)

Comment thread target/scripts/helper-functions.sh Outdated
Comment thread target/scripts/helper-functions.sh Outdated
georglauterbach
georglauterbach previously approved these changes Sep 9, 2021
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@georglauterbach
Copy link
Copy Markdown
Member

I'm fine with disabling the unreliable tests :)

polarathene
polarathene previously approved these changes Sep 9, 2021
@georglauterbach
Copy link
Copy Markdown
Member

Waiting for #2177, then rebasing, then merge this :)

@polarathene
Copy link
Copy Markdown
Member

Is this something that should be mentioned in docs at all beyond the env?

How is this FAQ entry affected?

@georglauterbach
Copy link
Copy Markdown
Member

I think this is backend logic and it is not absolutely necessary to mention it. If one would like to do it, I'm fine with it as well.

@georglauterbach
Copy link
Copy Markdown
Member

georglauterbach commented Sep 11, 2021

@NorseGaud feel free to merge this PR when tests pass and you think you have completed the checklist above :)

@georglauterbach georglauterbach added this to the v10.2.0 milestone Sep 11, 2021
casperklein
casperklein previously approved these changes Sep 11, 2021
Comment thread target/scripts/helper-functions.sh Outdated
@NorseGaud NorseGaud dismissed stale reviews from casperklein and polarathene via 8290bcc September 11, 2021 13:17
@github-actions
Copy link
Copy Markdown
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 5ec465a

@polarathene polarathene merged commit c267d8a into docker-mailserver:master Sep 11, 2021
@NorseGaud NorseGaud deleted the better-hostname-check branch September 11, 2021 14:20
@NorseGaud
Copy link
Copy Markdown
Member Author

Thanks y'all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants