fix: check-for-changes.sh should not fall out of sync with shared logic
#2260
Conversation
`/etc/postfix/sasl_passwd` is removed upfront if `SASL_PASSWD` is not set and this file exists for some reason. Otherwise, the intention was to add that ENV value to an empty `/etc/postfix/sasl_passwd`, thus `>` is appropriate here. No need for separate conditionals.
- Pulled out the warning conditional. - Added comment for context about the tabbed white-space chars. - Extracted the shared `/etc/postfix/sasl_passwd` adjustments to be a fix.
Rather than bloat `helper-functions.sh`, a new `relay.sh` file will make the various logic available as scoped functions. This is necessary for `check-for-changes.sh` to adopt.
Moves the conditional ENV into the merged method.
DRY to avoid falling out of sync again via co-location and shared methods.
These are possibly more verbose than needed and can be reduced at a later stage. They are helpful during this refactor process while investigating that everything is handled correctly.
Similar to other methods, explicitly clear `/etc/postfix/vhost` upfront. Avoids any lingering state if container mishaps from happening, such as container restarts causing issues in the past. Additional notes added for context.
`check-for-changes.sh`: - TODO for considering LDAP support/compatibility with change detection, at least for cert renewals. - Another note about potential LDAP support in future, where vhost step needs to make sure to persist a value specifically handled for LDAP. - In `start-mailserver.sh` the account creation is guarded by `SMTP_ONLY` ENV, carrying that over to this too. `helper-functions.sh`: Add the new `helpers/` scripts for other consumers to utilize. `accounts.sh`: - Add TODO for PR that needs to sync changes to this file after merge. - Add note regarding potential bug for bare domain setups with `/etc/postfix/vhost` and `mydestination` sharing same domain value. `aliases.sh`: - Add note about LDAP and new logic that will also operate on the `postfix-virtual.cf` file once PR is merged. - Split `_create_aliases` method into three separate responsibilities it covered, kept original method for other scripts to call. `relay.sh`: - Remove the tabs for a single space delimiter, revised associated comment. - Add PR reference for original `_populate_relayhost_map` implementation which has some useful details. - Remove unnecessary shellcheck disable rule.
This should avoid having to mess with any merge conflicts. Once the other PR is merged, the intended changes can be applied again here, and the `accounts.sh` method synced.
georglauterbach
added
area/scripts
kind/improvement
|
This PR look like it introduces really nice changes! Keep it up :D |
PR merged, used merge commit to align with master and now it's good to restore changes for this method, along with syncing the changes from the PR it was waiting on. Most of that PR has been assigned to a function to scope it, rather than blend in with the main account function logic.
Better to source through a single file instead. Relative paths should be ok to use too, instead of the absolute container paths.
polarathene
force-pushed
the
fix/refactor-check-for-changes
branch
from
bb07da1
to
bf349d1
Compare
This took a while to figure out, added some inline doc comments to assist others in future.
The helpers folder doesn't get copied over, add it afterwards.
This was missed when extracting from `check-for-changes.sh`, without it, results in duplicate entries (old + new) in the relay test causing a failure.
Correct 👍
Yeah, an initial pass. It still causes a large diff so I didn't want to introduce any broader changes, those can be addressed in a follow-up PR at a later point :) I made sure to use commit history for easier to follow transition of changes if helpful. |
|
Sorry for the delay. Sometimes I am bit scared when I see large PR/changes 😄. I'll try to review it within the next week. |
It's really not as large as it looks in the diff. The commit history is intended to help provide you with that confidence as I was careful to break down the transition for better review by diffs. Even without that, you can bring up your own diff app (eg: meld) and take the
No worries, take your time reviewing it, no rush 👍 |
Failure occurred from a merge commit, so the default range is too sensitive for this test.
|
One other thing to consider is, to conditionally source some helpers, e.g.: source SASL stuff only, if SASL is used |
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Are you referring to the That is something I'd like to do, but deferred from this PR to ease review since I wanted it to be reasonably clear in the diff that the bulk of it is just shifting the logic out to new files, especially with |
|
I was referring to |
Ah, well that's already handled either within the function or via the caller IIRC. The SASL one has two functions, one that checks the ENV and another that checks for a file. It seems that those conditions are best suited where they are and that conditionally sourcing the functions/file would be a bit more messier and prone to timing? (especially at setup) |
A bit. But it will also save memory 😉 In that particular case, I admit, it's not worth 😆 Edit: I misunderstood the timing thing. There should be no race condition or what did you mean? |
Timing wise, I was thinking more about the time that Just seems like more maintenance burden for minimal benefit vs just sourcing it all and guarding with conditionals within the respective files like we presently have. There is some conditionals external still, such as Personally I prefer the requirements to run the logic to be clearly defined within these files themselves, so conditionally sourcing at After this is merged, I'd like to move some of the So at best I might leave a comment to reference them at the top, but rely on importing via |
Co-authored-by: Casper <casperklein@users.noreply.github.com>
Description
Removes duplicate logic from
check-for-changes.shthat is used/maintained elsewhere to avoid risk of problems, as this code is already starting to diverge / rot.Previously the change detection support has had code added for rebuilding config upon change detection which is the same as code run during startup scripts. Unfortunately over time this has fallen out of sync. Mostly the startup scripts would get maintenance and the contributor and reviewers may not have been aware of the duplicate code handled by
check-for-changes.sh.That code was starting to diverge in addition to some changes in structure (eg: relay host logic seems interleaved here vs separated out in startup scripts). I wanted to address this before it risks becoming a much bigger headache.
Rather than bloat
helper-functions.shfurther, I've added ahelpers/folder extracting relevant common logic between startup scripts andchangedetector. If you want to follow that process I've kept scoped commits to make those diffs easier. Some minor changes/improvements were added but nothing significant.I've not given too much thought to naming conventions at present, that should still be fairly flexible. A similar PR will be handled for TLS functionality
and additional changes tocheck-for-changes.shonce #2245 is merged. I want to minimize any merge conflicts due to related PRs open.accounts.shcontains logic that #2248 is updating, so I've reverted mysetup-stack.shchanges for that affected method to avoid merge conflicts. Once it is merged, a merge commit into this PR should go smoothly and I'll sync changes toaccounts.sh, followed by bringing back the minimalsetup-stack.sh:_setup_dovecot_local_usermethod this PR introduces.Ran into a little confusion with
shellchecklint, that's been resolved and some extra maintainer comment docs in thelint.shforshellcheckto clarify it to others. Probably will migrate to maintainer docs at a later stage.A recent PR
shellchecklinting behaviour/config came up and regardingSCRIPTDIRwas also identified here.Type of change
Checklist:
docs/)