chore(housekeeping): Create a helper for change detection functionality#2610
Merged
polarathene merged 10 commits intodocker-mailserver:masterfrom Jun 4, 2022
Merged
Conversation
This doesn't really belong in `helpers/ssl.sh`. Moving to it's own helper script.
It seems relevant to migrate the related support during startup for the change detection feature into this helper. I opted to move the call from `start-mailserver.sh` into the `_setup` call at the end for a more explicit/visible location.
It belongs there, not in `helpers/index.sh`.
polarathene
added
priority/low
area/scripts
kind/improvement
Presently `test/test_helper.bats` is using it's own `CHKSUM_FILE` instead of sourcing the var for the filepath. `test_helper/common.bash` was calling a method to check for changes, but this helper may not correctly detect letsencrypt related changes as these are not ENV rely on, but global vars handled by `helpers/dns.sh`, so that should be run first like it is for `check-for-changes.sh`.
The linter misunderstood, we want to interpolate the var from within the container.
georglauterbach
approved these changes
May 31, 2022
casperklein
approved these changes
Jun 4, 2022
6 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
No functionality changes. Housekeeping change to better organize the supporting change detection functionality by co-locating to a single helper script.
Reasoning and changes covered
The initial PR that split up the original
helper-functions.shinto separate helper scripts movedCHKSUM_FILEglobal var intohelpers/index.shand_monitored_files_checksums()intohelpers/ssl.sh. It seemed more appropriate to migrate these into their own helper script.I assume @georglauterbach kept
CHKSUM_FILEinhelpers/index.shso it would not matter what order the scripts are sourced in, which is a valid concern. As we're not using it anywhere else with the introduction of this new helper, I think that is no longer a concern?I have additionally extracted out the related support from
setup-stack.sh.start-mailserver.shwill still callsetup-stack.sh:_setupwhich now calls the relevant helper method (with a revised comment). This seemed a better place to locate the call, but I'm open to reverting that.Besides
check-for-changes.sh, the helper makes functionality available to container startup scripts to initialize theCHKSUM_FILEcontent once container setup scripts complete, and co-locates everything thecommon.bashtest helper methodwait_for_changes_to_be_detected_in_container()needs for it's functionality.Type of change
Checklist: