FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

docs: Remove ENV `ONE_DIR` by polarathene · Pull Request #3840 · docker-mailserver/docker-mailserver · GitHub

/ docker-mailserver Public

docs: Remove ENV ONE_DIR - #3840

Merged
polarathene merged 6 commits into
docker-mailserver:masterfrom
polarathene:feat/remove-env-one-dir
Jan 28, 2024
ConversationCommits (6)ChecksFiles changed
Merged

docs: Remove ENV ONE_DIR#3840
polarathene merged 6 commits into
docker-mailserver:masterfrom
polarathene:feat/remove-env-one-dir

Conversation

polarathene commented Jan 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Description

I don't see this as a breaking change given the logic we have for it.

  • I have brought this up a few times in the past, this PR finally contributes the suggestion to remove ONE_DIR.
  • It doesn't really serve a purpose anymore, the /var/mail-state location existing is the real detection for triggering the runtime-state consolidation feature.
  • I've updated all docs referencing it, and the first commit improves the documentation on our volumes (I can split this out into a separate PR if preferred, although the ONE_DIR changes in docs relates to centralizing the docs for our DMS state volume).

I've explained DMS Volumes several times in past issues (example), so that should be a helpful addition to reference to link users to instead 👍

Type of change

  • Improvement (non-breaking change that does improve existing functionality)
  • 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/)
  • New and existing unit tests pass locally with my changes
  • I have added information about changes made in this PR to CHANGELOG.md

polarathene added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation area/documentation labels Jan 27, 2024
polarathene added this to the v14.0.0 milestone Jan 27, 2024
polarathene self-assigned this Jan 27, 2024
polarathene commented Jan 27, 2024
View reviewed changes
Comment thread CHANGELOG.md Outdated
polarathene force-pushed the feat/remove-env-one-dir branch from 34f222d to c146b76 Compare January 27, 2024 03:32
polarathene force-pushed the feat/remove-env-one-dir branch from c146b76 to d14db98 Compare January 27, 2024 03:37
chore: Remove ONE_DIR ENV from scripts
e959417 
Only `ONE_DIR=0` has any effect. As the actual feature is now dependent upon the `/var/mail-state` location existing.

It is advised not mounting anything there instead if wanting to avoid runtime state consolidation.
docs: Adjust link ref convention
3eba74d 
This is more search friendly / organized to find references to all DMS volumes.
polarathene force-pushed the feat/remove-env-one-dir branch from d14db98 to 3eba74d Compare January 27, 2024 03:52

polarathene commented Jan 27, 2024

Copy link
Copy Markdown
Member Author

Just some thoughts on the lint failure as I didn't expect this (feel free to ignore this comment):

  • It's only because I modified the last line in VSCode, while other files saved didn't adjust it.
  • This contributes to a prior confusion about "final newline" being a line-ending only, but in VSCodes case the default is probably where I got the idea that it required a full blank line 😓

# 0 for missing final newline, 1 for present:
$ tail -c 1 docs/content/config/environment.md | wc -l
0

We already have that prevented by a recent PR for user config parsing (where the last report was related to VSCode defaults), and the ECLint check config catches it here in PRs.

Perhaps a git pre-commit hook could better prevent it (I haven't checked if that's implicit, or requires the contributor to use an editor plugin like for .editorconfig), there doesn't seem to be an equivalent .gitattributes feature like we do for preventing CRLF line-endings.

I'm wondering if there's any value in highlighting that default setting for VSCode in our docs (with required configuration change) 🤔

VSCode otherwise expects a blank line (a separate setting enabled by default optionally also renders it as a separate blank line visually):

lint: Ensure final newline is present
90dcb67 
VSCode by default excludes this if the last line rendered is removed (rendered as a separate blank line).

A separate setting can enforce adding the final newline upon save regardless.

georglauterbach commented Jan 27, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

I have Insert Final Newline set to true, but I didn't know about Render Final Newline (which is set to dimmed for me by default). IMO

"files.trimTrailingWhitespace": true

is useful as well.

georglauterbach approved these changes Jan 27, 2024
View reviewed changes

georglauterbach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I like the first commit in particular; it always bothered me how we only document the volumes in the FAQ 👍🏼

polarathene commented Jan 27, 2024

Copy link
Copy Markdown
Member Author

I'll leave this a little bit longer for @casperklein to possibly chime in since I recall ONE_DIR discussions with him in the past too.

polarathene mentioned this pull request Jan 28, 2024
Closed
1 task

casperklein commented Jan 28, 2024

Copy link
Copy Markdown
Member

Short and sweet:

  • Let's remove ONE_DIR 👍
  • I haven't enough time to think this completely through. If there are no drawbacks for people, currently still using ONE_DIR, then it's fine to merge. Otherwise I would wait for the next major release.

I am also using this in VSCode:

    "files.trimTrailingWhitespace": true,
    "files.insertFinalNewline": true,

georglauterbach commented Jan 28, 2024

Copy link
Copy Markdown
Member

If there are no drawbacks for people, currently still using ONE_DIR, then it's fine to merge. Otherwise I would wait for the next major release.

I can't think of any issues this PR introduces in this regard; it would be a problem if someone mounted a volume to the paths we specify in our docs but then sets ONE_DIR=0, which makes no sense really. IMO this is fine to go into v14.0.0, but I'll leave it up to you :)

github-actions Bot commented Jan 28, 2024

Copy link
Copy Markdown
Contributor

Documentation preview for this PR is ready! 🎉

Built with commit: 41359fe

polarathene merged commit 3b11a83 into docker-mailserver:master Jan 28, 2024
AlMaVizca added a commit to AlMaVizca/docker-mailserver-helm that referenced this pull request Jun 19, 2024
ONE_DIR has been removed
92da921 
PR - docker-mailserver/docker-mailserver#3840

Signed-off-by: Aldo Maria Vizcaino <aldo.vizcaino87@gmail.com>
AlMaVizca added a commit to AlMaVizca/docker-mailserver-helm that referenced this pull request Jun 20, 2024
ONE_DIR has been removed
432e122 
PR - docker-mailserver/docker-mailserver#3840

Signed-off-by: Aldo Maria Vizcaino <aldo.vizcaino87@gmail.com>
polarathene mentioned this pull request Jun 21, 2024
Closed
1 task
polarathene mentioned this pull request Jan 17, 2025
Closed
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

casperklein casperklein approved these changes
georglauterbach georglauterbach approved these changes

Assignees

polarathene

Labels

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

Projects

None yet

Milestone

v14.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL