Skip to content

Complete refactoring for start-mailserver.sh#1605

Merged
georglauterbach merged 4 commits intodocker-mailserver:masterfrom
georglauterbach:start_mailserver
Sep 23, 2020
Merged

Complete refactoring for start-mailserver.sh#1605
georglauterbach merged 4 commits intodocker-mailserver:masterfrom
georglauterbach:start_mailserver

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Sep 10, 2020

The Second Half

This one is rather a blue whale. I completely refactored start-mailserver.sh. Therefore, it is now included into shellcheck. This was indeed a slow decend into madness.

Changes

  • completely refactored start-mailserver.sh
  • adjusted Makefile to check this script too

To Do

Post Scriptum

@erik-wramner I accidentally merge-commited all commits from #1601; I will squash them next time. I'm sorry for the inconvenience.

Georg Lauterbach added 2 commits September 8, 2020 19:50
@erik-wramner
Copy link
Copy Markdown
Contributor

I have this on my TODO list, I'll try to find time in the weekend.

* removed unnecessary shellcheck comments adding braces and "" where necessary
* corrected some mistakes in CONTRIBUTING
* Makefile now uses correct shellcheck
@georglauterbach georglauterbach changed the title WIP: Complete refactoring for start-mailserver.sh Complete refactoring for start-mailserver.sh Sep 10, 2020
@georglauterbach
Copy link
Copy Markdown
Member Author

any news on your side @erik-wramner ? @fbartels have you had time?

Copy link
Copy Markdown
Member

@fbartels fbartels left a comment

Choose a reason for hiding this comment

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

Sorry, quite a large changeset and therefore not that easy to review. But in the end it looks like almost exclusively formatting changes and no functional changes (at least I cannot remember seeing one of those).

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 18, 2020

That's correct @fbartels. In the end, it's only a refactoring:) The only "major" changes are those where I substituted code, for example

array=$(cat $THIS | tr ":" "\n")

which is ugly and not robust became

declare -a ARRAY
IFS=':'
read -r -a ARRAY < <(cat "${THIS}")

which is more verbose, but read was made for exactly this case, therefore being robust.

There were similar changes here and there. The majority of line changes came from brace substitutions and "" around variables.

PS: Thank's for approving.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 21, 2020

@erik-wramner I see you have very little time at hand, plus the PR is pretty big. I really understand this and I don't want to force you into reviewing these changes. If it's okay with you, just tell me: @fbartels already approved and the tests look good, so there should be no risk involved in merging this if you are okay with not having reviewed this one. What's your take on this?

There is one more thing I'd really like to see: A new release. You already drafted one for 7.0.1. This should go alongside a newer stable version. The current :stable-tag for Docker is 4 month old.

@erik-wramner
Copy link
Copy Markdown
Contributor

I've approved the changes. I'll try to make a release this weekend provided that master is stable (i.e. no serious bugs reported).

@georglauterbach georglauterbach merged commit 566eaa0 into docker-mailserver:master Sep 23, 2020
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.

3 participants