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

sed wrapper #2158

Merged
merged 15 commits into from
Sep 5, 2021
Merged

sed wrapper #2158

merged 15 commits into from
Sep 5, 2021

Conversation

casperklein
Copy link
Member

@casperklein casperklein commented Aug 29, 2021

Description

This introduces sedfile, a wrapper for sed -i commands.

Problem: When sed -i is used to manipulate files, sed exit with error code 0, even when the file was not altered:

$ cat foo
foo

$ sed -i 's/hello/world/g' foo
$ echo $?
0

$ cat foo
foo

sedfile compares the file hash before and after the sed operation and fails, if the file was not altered (hashes do match).

$ sedfile -i 's/hello/world/g' foo
Error: sed -i s/hello/world/g foo

$ echo $?
1

This helps to prevent issues caused by failed file modifications, e.g. a package is upgraded and the config file structure changes.

Identified issues

imap_idle_notify_interval

Error: sed -i s/#imap_idle_notify_interval = 2 mins/imap_idle_notify_interval = 29 mins/ /etc/dovecot/conf.d/20-imap.conf
https://github.com/docker-mailserver/docker-mailserver/pull/2158/checks?check_run_id=3455478612#step:6:2236

Reason: imap_idle_notify_interval is not present in /etc/dovecot/conf.d/20-imap.conf.
Action: Line deleted

⚠️ Is this entry needed, should it be re-added?
Edit: #554
Edit2: Broken since commit: added dovecot quota feature

adapt mkcert for Dovecot community repo (old leftover)

Error: sed -i s/CERTDIR=.*/CERTDIR=\/etc\/dovecot\/ssl/g /usr/share/dovecot/mkcert.sh
Error: sed -i s/KEYDIR=.*/KEYDIR=\/etc\/dovecot\/ssl/g /usr/share/dovecot/mkcert.sh
Error: sed -i 's/KEYFILE=.*/KEYFILE=\$KEYDIR\/dovecot.key/g' /usr/share/dovecot/mkcert.sh
https://github.com/docker-mailserver/docker-mailserver/runs/3455521780?check_suite_focus=true#step:6:2225

Reason: CERTDIR, KEYDIR, KEYFILE already have the correct values in /usr/share/dovecot/mkcert.sh.
Fix: Affected lines removed from Dockerfile

Future to-do

Replace existing sed -i with sedfile -i in scripts and tests.

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 Aug 29, 2021
@casperklein casperklein added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low labels Aug 29, 2021
@casperklein casperklein added this to the v10.2.0 milestone Aug 29, 2021
@casperklein casperklein marked this pull request as ready for review August 29, 2021 17:24
@casperklein casperklein requested a review from a team August 29, 2021 17:24
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 👍🏼

@casperklein
Copy link
Member Author

@egavrilov Your addition from #554 broke some time ago. Do you know, if it's still needed nowadays?

@casperklein casperklein requested a review from a team September 1, 2021 16:25
wernerfred
wernerfred previously approved these changes Sep 1, 2021
@egavrilov
Copy link
Contributor

@casperklein hi there. It's not really a hard requirement to have idle time set from default to whatever value, but just a recomendation to have it higher than default because of battery drain on non-optimised client. I guess it is even possible to remove it because the problem is on the client actually.

@casperklein
Copy link
Member Author

I guess it is even possible to remove it because the problem is on the client actually.

It is already removed (unintentionally) since ~1.5 years. My question just was, if it's worth to re-add it. Now, I don't think so.
If someone needs that, it can still be configured in a custom dovecot config.

@casperklein casperklein enabled auto-merge (squash) September 4, 2021 18:44
@casperklein casperklein requested a review from a team September 5, 2021 21:45
@casperklein casperklein merged commit e89ea31 into docker-mailserver:master Sep 5, 2021
Copy link
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

LGTM.

Although the sed lines look a bit difficult to read some times, especially that dovecot one with all the path escaping \/ mixed in with the / sed delimiter. In other scripts we switched to a consistent | sed delimiter instead.

Not required for this PR but probably a worthwhile change :)

NorseGaud pushed a commit to NorseGaud/docker-mailserver that referenced this pull request Sep 17, 2021
* sed wrapper 'sedfile' added

* formatting

* sed --> sedfile

* typo

* fix lint

* debug

* fixme

* mkcert fix

* style adjusted

* Update Dockerfile
NorseGaud pushed a commit to NorseGaud/docker-mailserver that referenced this pull request Sep 17, 2021
* sed wrapper 'sedfile' added

* formatting

* sed --> sedfile

* typo

* fix lint

* debug

* fixme

* mkcert fix

* style adjusted

* Update Dockerfile
@casperklein casperklein deleted the sedfile branch September 28, 2021 00:49
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

5 participants