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

refactoring: split helper functions into smaller scripts #2420

Merged
merged 27 commits into from
Feb 21, 2022

Conversation

georglauterbach
Copy link
Member

@georglauterbach georglauterbach commented Feb 18, 2022

Description

This PR does not change any functionality of DMS, it just refactors the helper-function.sh script into smaller scripts in the earlier created helpers/ directory. helper-function.sh was a mess and all over the place. Now, a single script index.sh (which was already created before this PR) sources all helper functions, or one can individually source smaller helpers, somewhat sorted. Again, this is just a refactoring without functionality changes :)

One more thing: I changed . to source. As described in the commit message, source is idiomatic to Bash, and I find it easier to understand that a script is sourced here. I hope this is not a major problem for someone :)

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 SSL configuration function is rather large. Outsourcing it provides
the opportunity to run the function itself without sourcing the whole
`setup-stack.sh`.
Some difficulties on the way when it comes to changing the
certiicate files led to another approach. The file is briefly changed on
the host to see whether the changes are picked up.
Due to the changes in #2401 and the chages introduced here,
`check-for-changes.sh` needs just a little bit longer.
The function now only computes what it ought to, and not more. This
makes it a bit more efficient and less prone to problems.
The functions concerned with SSL/TLS setup were moved to
`target/scripts/helpers/ssl.sh` because they actaully belong there. This
is a non-issue as they are properly sourced by `index.sh` in
`target/scripts/helpers/` anyway. This is a proper seperation of
concerns.
This commit reverts a change previously introduced (that may got us some
false positives). Furthermore, a link was added for a "TODO" that can be
solved more easily in the future once these changes are resolved.

And the line splitting is now less aggressive.
The function was revised again for better maintainability and
readability. A second "staging" array was introduced which does a bit of
the heavy lifting when it comes to adding the correct files for
detecting changes. A missing `+` was added.
The `helper-functions.sh` script shall be splitted in the future. These
changes take the first step in splitting the file for SSL related
functions.
`source` can be used like `.`, but is idiomatic to Bash. I think it
offers better readability though, and one instantly recognizes that a
script is sourced here.
@wernerfred
Copy link
Member

TODO (I will add a proper description in a few minutes / hours) :D

Maybe put it as draft then 😄

@casperklein
Copy link
Member

casperklein commented Feb 18, 2022

TODO (I will add a proper description in a few minutes / hours) :D

Just mark it as draft, until finished 😉 That way it's easier for the other maintainer to know, when to start reviewing.

Edit: @wernerfred was faster 😆

@casperklein casperklein marked this pull request as draft February 18, 2022 12:22
@georglauterbach
Copy link
Member Author

I was just heading for lunch, the PR itself is finished and ready for review (I just didn't have enough time to write a proper description) 😆😆😆 That's why I did not mark it as a draft :)

@georglauterbach georglauterbach self-assigned this Feb 18, 2022
@georglauterbach georglauterbach added area/scripts kind/improvement Improve an existing feature, configuration file or the documentation priority/low priority/medium labels Feb 18, 2022
@georglauterbach georglauterbach added this to the v10.5.0 milestone Feb 18, 2022
target/scripts/helpers/error.sh Outdated Show resolved Hide resolved
target/scripts/helpers/network.sh Outdated Show resolved Hide resolved
Copy link
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.

.

@polarathene
Copy link
Member

polarathene commented Feb 19, 2022

I was under the impression that you usually want a blank line at the end of a file?

EDIT: There's a few examples here for why you might want to keep empty line at end of files.

Similar reason covered here noting many editors with feature to ensure the empty line on file save. And another citing roughly the same thing in a python community, but uses readline as an example.

I don't quite get the benefit of dropping them, but keen to know if there's a reason beyond preference 😅

FWIW, our own lint rules seem to prefer the empty line at end of file:

insert_final_newline = true


In files I've worked on, I also have a habit of adding a double blank to better separate context/bounds visually, such as big functions which have their own single blank lines in the function body. That's just a personal preference thing so I don't mind that it was stripped, consistency is all good 👍

I tend to do similar for tests too, such as in the letsencrypt one I think I used it to group related functions and tests, although a recent PR may have also stripped that away.

@casperklein
Copy link
Member

casperklein commented Feb 19, 2022

I think you mixed things up. There is a difference between a final new line, which as you stated is best-practice. But there is no good reason for an empty line.

Here is an example, that I hope helps to clarify things:

This writes hello with a final newline (0a in HEX) to the file world.

# echo hello > world
# hd world
00000000  68 65 6c 6c 6f 0a                                 |hello.|

This adds an additional empty line to the file world:

# echo >> world
# hd world
00000000  68 65 6c 6c 6f 0a 0a                              |hello..|

Notice the two 0a 0a at the end.

For completeness, here is a bad example:

This writes hello without a final newline (missing 0a at the end) to the file world.

# echo -n hello > world
# hd world
00000000  68 65 6c 6c 6f                                    |hello|

@casperklein
Copy link
Member

casperklein commented Feb 19, 2022

Damn it. The web UI lied to me. I just verified the files and all is already fine (final new line, not empty lines).

@polarathene
Copy link
Member

I think you mixed things up. There is a difference between a final new line, which as you stated is best-practice. But there is no good reason for an empty line.

Thanks for the clarification. It always seemed to render as an empty line to me in editors when I noticed it adding such on save. I guess that varies by editor presentation 😅

@georglauterbach
Copy link
Member Author

georglauterbach commented Feb 20, 2022

Damn it. The web UI lied to me. I just verified the files and all is already fine (final new line, not empty lines).

Which of your suggestions should be commited now? I guess the one about errors.sh, but the other ones as well?

@casperklein
Copy link
Member

Only the three unnecessary lines in error.sh. The rest is fine IMO. The webui pranked me into thinking, that there were empty lines. But there weren't.

I thought I deleted all wrong suggestions with the exception of one, but there were still two left. I deleted them now as well. So there is now only one wrong suggestion left, but only for the record, so that the following comments of polarathene and me makes sense.

@georglauterbach
Copy link
Member Author

georglauterbach commented Feb 20, 2022

I realized that indeed there were two 0a's at the end of the files (not sure why the web-interface would maybe show something different). I removed them from the scripts - now all should be well:

$ for FILE in * ; do hd $FILE | tail -n 2 | head -n 1 ; done
00001640  20 66 69 0a 7d 0a                                 | fi.}.|
00000a30  69 61 73 65 73 5f 63 6f  6e 66 69 67 0a 7d 0a     |iases_config.}.|
00000bc0  0a 7d 0a                                          |.}.|
000009c0  6b 69 6c 6c 20 31 0a 7d  0a                       |kill 1.}.|
000003d0  63 72 69 70 74 73 0a                              |cripts.|
00000530  66 69 0a 7d 0a                                    |fi.}.|
00000380  0a 7d 0a                                          |.}.|
00000360  2a 2f 7d 22 0a 7d 0a                              |*/}".}.|
00000aa0  74 61 6c 69 61 73 2e 31  2e 68 74 6d 6c 0a        |talias.1.html.|
00001040  0a 20 20 66 69 0a 7d 0a                           |.  fi.}.|
000001a0  20 66 69 0a 7d 0a                                 | fi.}.|
000056b0  7d 0a                                             |}.|
000000e0  0a 7d 0a                                          |.}.|

@casperklein
Copy link
Member

Interesting. When I checked out the branch to verify, all looked good. But maybe I did some mistake by doing that. I don't know. Then the web UI didn't lie to me in the first place 😆

@georglauterbach georglauterbach merged commit b61dfe1 into master Feb 21, 2022
@georglauterbach georglauterbach deleted the feature/split-helper-functions branch February 21, 2022 10:56
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/low priority/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants