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

Rename config examples directory #2438

Merged
merged 5 commits into from
Mar 2, 2022

Conversation

casperklein
Copy link
Member

@casperklein casperklein commented Feb 26, 2022

Description

To align with the documentation, this PR relocates the config directory.

See also discussion here.

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

@casperklein casperklein self-assigned this Feb 26, 2022
@casperklein casperklein added area/configuration (file) kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 26, 2022
@casperklein casperklein added this to the v11.0.0 milestone Feb 26, 2022
@casperklein casperklein mentioned this pull request Feb 26, 2022
@casperklein casperklein requested a review from a team February 26, 2022 18:32
polarathene
polarathene previously approved these changes Feb 26, 2022
@georglauterbach
Copy link
Member

georglauterbach commented Feb 26, 2022

Why the file mode changes (chmod -x) on the helper files? (I know this is a draft, just asking right away.)

@polarathene
Copy link
Member

Since there was a request to continue discussion here..

@casperklein said:

PS: I find the new directory structure still weird. docker-data/dms suggest, that docker-data is a general docker directory, with DMS as one of many projects. In reality, I doubt, that someone will ever use this directory to place other docker-data in, beside DMS. Why would anyone want to mix-up his DMS stack with data not related to DMS.

While I understand the reasons for the renaming (easier handling of our docs), personally I find it strange to have the proposed directory structure in this repo.

As stated, it was an improvement to our docs with the normalization effort for consistency. If you grep the docs, you'll find other directories beyond dms used in docker-data.

This provides a root data/volume directory with sub-folder for each container, thus related data, but still isolated by associated container. I use this approach for storing data with multi-container projects as littering the volume dirs one level higher in a project structure isn't helpful.


@georglauterbach said:

I agree with @casperklein that the directory structure proposed in our docs is, at least IMHO, not likely to be found out there in the wild (i.e. a bit weird).

Like I replied above to casper, this is a structure I use and makes sense to me, and our docs to avoid any vagueness.

I would assume that our users are familiar enough with Docker that if they did not like the docker-data dir and had no need for the extra layer due to no additional containers or isolation needs, that they'd adjust it accordingly. If our docs are not clear enough on that freedom to do so, and you feel that should be clarified better, go for it :)

Personally, I don't see much benefit in changing the paths used, especially since they are more useful in the current state for maintaining the docs vs before when paths were inconsistent or overlapping (containers from different images using same/similar paths like config/, or unclear if referencing directory path from host or container) across various doc pages.


I'm open to it being handled better too. I just remember the state of the docs prior to normalization due to various contributors over time and this sort of thing slipping through where it was harder to correct.

I'm not going to defend the current choice more than that.

If maintainers disagree with the justification and think it's fine/better to simplify the path, that's all good too 👍

@polarathene
Copy link
Member

polarathene commented Feb 26, 2022

Why the file mode changes (chmod -x) on the helper files?

See the commit message associated to them:

remove executable bit from scripts that are only sourced

@casperklein
Copy link
Member Author

Minor change, I don't wanted to open a separat PR. Now it's streamlined with scripts/startup/* which has the same file mods.

@casperklein
Copy link
Member Author

If you grep the docs, you'll find other directories beyond dms used in docker-data.

Okay, I wasn't aware of that. I took only the 4 paths from docker-compose.yml into consideration, when making my comment.

Going back to the previous approach (just using config and reverting the docs) is IMHO not an option. All the hard work on the docs from you would be useless.

But to avoid problems mentioned in #2436, the directory must be renamed. However, I am still not happy with the new long path.

What about renaming the config directory to config-samples? This way, you can also clone this repo and use setup.sh, but without headache like in #2436 + we avoid to have the long path in our repo.
People that want to use some or all of the example files, can easily copy them to their config directory.

@polarathene
Copy link
Member

Redundant

However, I am still not happy with the new long path

That's only really an option with the config/ folder in the repo rather than the docs right? Isn't that folder more of an example?

It doesn't have to match the docs path, in fact it may be more clearer and less surprising if it's clearly example/config/ instead?

Is there a benefit to implicitly using that folder when a user clones the repo and uses setup.sh at the project root? I don't think the user expected that either. example/config/ should make it clear that it's an example only and not actually required to run. Some users do seem to get confused about that, similar to mailserver.env.

..I should really read the full message before replying 😅


What about renaming the config directory to config-samples?

Yes that works just as well as example/config/ 👍

@casperklein
Copy link
Member Author

As long as there are no other subdirectorys, I would like to keep it flat.

What is better: config-samples or config-examples? I tend to the second one.

@polarathene
Copy link
Member

What is better

config-examples seems good 👍

Or if it's more of a single example rather config-example?

@casperklein
Copy link
Member Author

Or if it's more of a single example rather config-example?

There are various files in that directory. Plural is fine.

@casperklein casperklein marked this pull request as ready for review February 27, 2022 16:52
@casperklein casperklein changed the title relocate config sample directory ename config examples directory Feb 27, 2022
@casperklein casperklein changed the title ename config examples directory Rename config examples directory Feb 27, 2022
@georglauterbach georglauterbach added the meta/feature freeze On hold due to upcoming release process label Feb 27, 2022
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 👍

Due to the file adjustments this PR has to wait until 10.5.0 is released.

@casperklein
Copy link
Member Author

See milestone 😉

@georglauterbach georglauterbach removed the meta/feature freeze On hold due to upcoming release process label Mar 2, 2022
@casperklein casperklein enabled auto-merge (squash) March 2, 2022 22:53
@casperklein casperklein merged commit a8a8c85 into docker-mailserver:master Mar 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2022

Documentation preview for this PR is ready! 🎉

Built with commit: c183d40

@casperklein casperklein deleted the config-directory branch March 2, 2022 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration (file) kind/improvement Improve an existing feature, configuration file or the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants