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

HOSTNAME and DOMAINNAME setting improvements #2175

Merged
merged 11 commits into from
Sep 11, 2021

Conversation

NorseGaud
Copy link
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
…omain

doc: OVERRIDE_HOSTNAME takes priority
@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
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 :)

target/scripts/helper-functions.sh Outdated Show resolved Hide resolved
target/scripts/helper-functions.sh Outdated Show resolved Hide resolved
georglauterbach
georglauterbach previously approved these changes Sep 9, 2021
Copy link
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
Member

I'm fine with disabling the unreliable tests :)

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

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

@polarathene
Copy link
Member

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

How is this FAQ entry affected?

@georglauterbach
Copy link
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
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
target/scripts/helper-functions.sh Outdated Show resolved Hide resolved
@github-actions
Copy link
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
Member Author

Thanks y'all :)

NorseGaud added a commit to NorseGaud/docker-mailserver that referenced this pull request Sep 17, 2021
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).

---

* using _obtain_hostname_and_domainname helper + covers when not a subdomain
doc: OVERRIDE_HOSTNAME takes priority

* added tests for non-subdomain hostname + further improvements

* moved SRS DOMAINANME tests into hostname test file + Allowing DOMAINNAME ENV to override what would be automatically set

---

Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
NorseGaud added a commit to NorseGaud/docker-mailserver that referenced this pull request Sep 17, 2021
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).

---

* using _obtain_hostname_and_domainname helper + covers when not a subdomain
doc: OVERRIDE_HOSTNAME takes priority

* added tests for non-subdomain hostname + further improvements

* moved SRS DOMAINANME tests into hostname test file + Allowing DOMAINNAME ENV to override what would be automatically set

---

Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
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.

None yet

4 participants