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
scripts: split setup-stack.sh
#3115
Conversation
NO CHANGE TO FUNCTIONALITY, JUST COPYING
The variables script is actually kinda like a stack for us; what you additionally need to know is that the declaration of the arrays is weirdly scoped in Bash; hence #3112 takes such an unusual approach at placing the declarations. With variables being a stack, everything becomes a bit nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manually compared diffs before/after, LGTM 😅 👍
Cheers for handling this step as a separate PR 🎉
@@ -10,6 +10,7 @@ function _import_scripts | |||
source "${PATH_TO_SCRIPTS}/accounts.sh" | |||
source "${PATH_TO_SCRIPTS}/aliases.sh" | |||
source "${PATH_TO_SCRIPTS}/change-detection.sh" | |||
source "${PATH_TO_SCRIPTS}/dhparams.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This files contents could probably live in the ssl.sh
helper as it's related to that support (ciphers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this PR (to get it through), and will apply this feedback in the next, smaller PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3124
do | ||
# shellcheck source=/dev/null | ||
source "${FILE}" | ||
done < <(find /usr/local/bin/setup.d/ -type f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally bad practice - files including spaces for example will not be handled correctly. You should add -print0
to the find command and define a delimiter -d $'\0'
in the read
command or just use globbing instead:
for i in /usr/local/bin/setup.d/*.sh; do source "$i"; done
This should also be faster, because no external program needs to be run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require shopt -s globstar
since we need to use /usr/local/bin/setup.d/**/*.sh
, but I'm absolutely fine with that! I will provide a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3124.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I overlooked that there is also a subdirectory and not just files.
Description
Superseeds parts of #3112. Makes review easier. Split
setup-stack.sh
, did not add / remove any functionality. Basically a glorified copy-paste with minor renamings.Fixes issue where reviewing #3112 is a chore, I guess.
I will leave #3112 open so I can gradually implement all the changes this single PR would bring. Having reviewed #3112 is good because you basically know what's coming @polarathene, so no time wasted here.
The first commit is basically just copy-paste everywhere, the rest is more renaming/minor adjustments.
This PR is the first part of a series to come about refatoring some parts of the setup with the final goal of disallowing faulty restarts.
This PR is BIG, but actually 95% copy-paste; there are no changes in functionality whatsoever.
Type of change
Checklist:
docs/
)