Skip to content

scripts: refactoring & miscellaneous small changes#2499

Merged
georglauterbach merged 13 commits intomasterfrom
scripts/refactoring/miscellaneous
Mar 26, 2022
Merged

scripts: refactoring & miscellaneous small changes#2499
georglauterbach merged 13 commits intomasterfrom
scripts/refactoring/miscellaneous

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Mar 21, 2022

Description

Please see the commit messages. This is a housekeeping PR which includes several small changes.

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
  • 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

The changes are:

1. Replaced `""` wiht `''` where possible (reasoning: Bash is very
   implicit and I'd like to use `''` where possible to indicate no
   variables are expanded here)
2. `> /file` -> `>/file` according to our style guide
3. Some log adjustments for messages where I deemed it appropriate
4. Then, an error message from a Dovecot setup was also prevented (by
   adding a check whether the directory is present before a `: >...`
   command would create a file in this directory).

These are all small, miscellaneous changes that I wanted to combine into
one commit and ultimately one PR because I see no point in opening a PR
for every small change here. I hope this is fine.
This ensure the last log message is actually logged before Supervisor
logs the message that it received a SIGTERM. This makes reading the log
easier because now the causal relationship is shown (we are terminating
Supervisor, and not someone else and we're just logging it).

I forgot to replace `""` with `''` in `update-check.sh`, so I included
it here because this is the last commit before PR review.
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Mar 21, 2022
@georglauterbach georglauterbach added this to the v11.0.0 milestone Mar 21, 2022
@georglauterbach georglauterbach self-assigned this Mar 21, 2022
Copy link
Copy Markdown
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

You removed the logging of date/time. But that is essential.
I like to see in the log, when update-check runs for the last time.

Comment thread target/scripts/update-check.sh Outdated
@georglauterbach
Copy link
Copy Markdown
Member Author

You removed the logging of date/time. But that is essential. I like to see in the log, when update-check runs for the last time.

I re-added the timestamps to the log messages with the format from check-for-changes.sh which I added to utils.sh as a common function.

@casperklein
Copy link
Copy Markdown
Member

casperklein commented Mar 21, 2022

I re-added the timestamps to the log messages with the format from check-for-changes.sh which I added to utils.sh as a common function.

Instead of providing in each _log call $(_print_date), something more generic would be easier:

_log 'debug' "$(_print_date) Remote version information fetched" --> _log_date 'debug' "Remote version information fetched"

_log_date() {
  _log "$1" "$(_print_date)$2"
}

PS: As we take the output from the function and don't directly print it, _get_date sounds better to me.

Edit: Until otherwise needed, just include _print_date into _log_date():
_log "$1" "$(date "%Y-%m-%d %H:%M:%S")$2"

That way, only the log file needs to be sourced, instead of index.sh.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Mar 21, 2022

I re-added the timestamps to the log messages with the format from check-for-changes.sh which I added to utils.sh as a common function.

Instead of providing in each _log call $(_print_date), something more generic would be easier:

_log 'debug' "$(_print_date) Remote version information fetched" --> _log_date 'debug' "Remote version information fetched"

_log_date() {
  _log "$1" "$(_print_date)$2"
}

PS: As we take the output from the function and don't directly print it, _get_date sounds better to me.

Edit: Until otherwise needed, just include _print_date into _log_date(): _log "$1" "$(date "%Y-%m-%d %H:%M:%S")$2"

That way, only the log file needs to be sourced, instead of index.sh.

Will do 👍🏼

EDIT: Done in 0968953

The new function will log a message with a proper timestamp. This is all
handled in `log.sh`, we therefore not need to source other files too.

This will be used in the future by `check-for-changes.sh` as well :)
Comment thread target/scripts/helpers/log.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh
Comment thread target/scripts/update-check.sh Outdated
Comment thread target/scripts/update-check.sh Outdated
polarathene
polarathene previously approved these changes Mar 21, 2022
Copy link
Copy Markdown
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.

Not much to add, most of the concerns I marked up for review earlier was already caught and addressed by you both 😅

None of these changes are all that important, feel free to dismiss them.

Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Comment thread target/scripts/startup/setup-stack.sh Outdated
Co-authored-by: Casper <[email protected]>
Co-authored-by: Brennan Kinney <[email protected]>
Comment thread target/scripts/startup/setup-stack.sh Outdated
polarathene
polarathene previously approved these changes Mar 22, 2022
polarathene
polarathene previously approved these changes Mar 22, 2022
Comment thread target/scripts/update-check.sh Outdated
@georglauterbach georglauterbach merged commit 7721a48 into master Mar 26, 2022
@georglauterbach georglauterbach deleted the scripts/refactoring/miscellaneous branch March 26, 2022 09:17
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants