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

scripts: split setup-stack.sh #3115

Merged
merged 8 commits into from Feb 26, 2023
Merged

Conversation

georglauterbach
Copy link
Member

@georglauterbach georglauterbach commented Feb 25, 2023

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

  • 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

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.
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 25, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Feb 25, 2023
@georglauterbach georglauterbach self-assigned this Feb 25, 2023
Copy link
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.

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"
Copy link
Member

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)

Copy link
Member Author

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

See #3124

@georglauterbach georglauterbach merged commit f35b600 into master Feb 26, 2023
@georglauterbach georglauterbach deleted the scripts/split-setup.sh branch February 26, 2023 10:42
do
# shellcheck source=/dev/null
source "${FILE}"
done < <(find /usr/local/bin/setup.d/ -type f)
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #3124.

Copy link
Member

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.

georglauterbach added a commit that referenced this pull request Feb 26, 2023
georglauterbach added a commit that referenced this pull request Feb 26, 2023
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.

None yet

3 participants