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

refactor: Revise check-for-changes.sh #2615

Conversation

polarathene
Copy link
Member

@polarathene polarathene commented Jun 4, 2022

Description

This PR is mostly housekeeping for check-for-changes.sh, in preparation for further refactoring.

Changes have been staged via individual commits with messages for further context. It may be easier to follow the changes via commit diffs, than as a whole.

Change Overview:

  • Inline docs for check-for-changes.sh have been shuffled around and revised a bit.
  • Change processing extracted from the main change detection loop method to their own methods:
    • _get_changed_files() - I extracted the grep+sed line to it's own method.
      • This was due to the verbosity of inline docs I added. I value being able to under to know quickly at a glance what I'm looking at without having to go refresh on the syntax 😅 - However if other maintainers feel this is a bad idea it can be reverted, no worries!
    • _postfix_dovecot_changes() - The bulk of change processing was moved to this method. I've added conditionals to only run relevant logic.
    • _ssl_changes() - Just shifted, no logic changed. REGEX_NEVER_MATCH and ACME_CERT_DIR vars scope set to local.

Type of change

  • Improvement (non-breaking change that does improve existing functionality)

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
  • New and existing unit tests pass locally with my changes

@polarathene polarathene self-assigned this Jun 4, 2022
@polarathene polarathene added priority/high area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Jun 4, 2022
@polarathene polarathene added this to the v11.1.0 milestone Jun 4, 2022
@polarathene polarathene marked this pull request as draft June 4, 2022 10:46
@polarathene

This comment was marked as resolved.

Necessary for future commit diffs to not be unnecessarily noisy..
Clarifies what is going on (and how) without having to look it up. To reduce noise in the main logic loop, extracted to a separate method.
No logic changed, only shifted. `REGEX_NEVER_MATCH` and `ACME_CERT_DIR` changed to local scope vars.
The remaining change detection handling is grouped into a separate function.
Better document change dependencies.
@polarathene polarathene force-pushed the refactor/revise-checkforchanges branch from 8e89062 to eee8a5a Compare June 9, 2022 23:21
@polarathene polarathene removed the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Jun 9, 2022
@polarathene polarathene marked this pull request as ready for review June 9, 2022 23:26
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 👍🏼

@polarathene polarathene merged commit 851ec8c into docker-mailserver:master Jun 11, 2022
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/high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants