Skip to content

Fixed bugs and refactor of main file.#7

Open
HerissonMignion wants to merge 12 commits intospectrasecure:mainfrom
HerissonMignion:main
Open

Fixed bugs and refactor of main file.#7
HerissonMignion wants to merge 12 commits intospectrasecure:mainfrom
HerissonMignion:main

Conversation

@HerissonMignion
Copy link
Copy Markdown

Fixed bugs related to missing quotes when calling functions or programs. Refactor the main file and the argument parsing. Because arguments are parsed slightly differently, i've also rewritten the help message and adjusted the arise command in the github workflows.

Copy link
Copy Markdown
Member

@neonspectra neonspectra left a comment

Choose a reason for hiding this comment

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

Hey! Thank you for the large PR! Sorry for taking some time to review it; things are hectic for me so tech hobby stuff has been on the backburner for me.

I've worked my way through your contributions and requested some changes. Some thoughts, as well as comments since it appears that PR base branch modification is disabled:

  • I requested changes to keep the arise build syntax. More comments on this in the suggestions.
    • The main script refactor is really nice. It does technically introduce breaking changes since you can't mix args together with your implementation, but your argument handling is ultimately a lot cleaner than the hacky way I did it.
    • I tried to keep your changes while also preserving syntax that would most definitely break someone's deployment if pulling in a new main without being careful about it.
  • I really appreciate your rewrite of the help snippet. Can you update docs/guides/running-arise-locally to either reference the new information, or cut the old help snippet in there and simply request that the user reference /lib/functions/inline/arise_help.sh for the full information?
    • Doesn't matter to me which way; but i think it makes sense to update this doc as part of merging a PR with changes to the help syntax.
  • TY for the quote linting work. I really need to set up shellcheck on this repo at some point but it's low on my to-do list since this project works well enough for my personal website I'm using it for.

Hope these comments all make sense. Thanks again for your contribution!

##############################################################

cd "$(dirname $0)"
cd "$SCRIPT_DIR"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Set the default as an option so that a user can change the SCRIPT_DIR if they want to above; otherwise they leave it unset and it uses the default.

Suggested change
cd "$SCRIPT_DIR"
# Default to the script's directory if SCRIPT_DIR is not set
# https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script
SCRIPT_DIR_DEFAULT=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
# Check if SCRIPT_DIR is set and non-empty using parameter expansion; otherwise use SCRIPT_DIR_DEFAULT
cd "${SCRIPT_DIR:-$SCRIPT_DIR_DEFAULT}" || {
echo "Error: Unable to change directory to '${SCRIPT_DIR:-$SCRIPT_DIR_DEFAULT}'"
exit 1
}

Copy link
Copy Markdown
Author

@HerissonMignion HerissonMignion Jan 8, 2025

Choose a reason for hiding this comment

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

I think it's nice for the moment (and the forseable future?) that arise is small enough that it comes with the website that it builds, kind of like the way the gnu autotools are "installed" in the project they compile (but without the complexity of gnu autohell autotools). (EDIT: And therefore I dont see the point of making this configurable.)

But if you insist, I'll do as you say.


arise_logo() {
cat <<EOF
cat <<"EOF"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no reason to enable variable expansion here

Suggested change
cat <<"EOF"
cat <<EOF

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

putting it in quotes is what disables variable expansion

Comment on lines +3 to +4
# https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the reason you modified this and moved it up here is to allow a config option to make it easier to overwrite To make this cleaner, I changed it to an optional item; see additional suggestion below to use the snippet by default but allow overwrite.

Since PR base branch changes aren't disabled and GitHub reviews won't let me comment on the entire file, can I ask that you move this option below the version tag stub but above the 'begin main function' stub?

Suggested change
# https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
# If you want to manually specify the path to the arise binary, you can uncomment and modify the variable below.
# This should not be necessary in most cases and this should generally be left as default.
# SCRIPT_DIR="/change/this/to/path/to/arise"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The reason I added this is because the script(s) is very reliant on relative paths to the root of the arise folder. My rationale is that it doesn't hurt to begin with a cd to the directory of the arise (and I get the path via this line) instead of relying on the user or external factors to have the appropriate current directory.

The reason this is at the top of the file is because that stackoverflow answer that I always copy paste in such situation says to be aware that this must be done before cd-ing to any other directory, so i paste it on the first lines.

HerissonMignion and others added 10 commits January 7, 2025 21:23
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
Co-authored-by: Neon <10284121+neonspectra@users.noreply.github.com>
@HerissonMignion
Copy link
Copy Markdown
Author

Well, I think I addressed most of your concerns. I implemented support for combined options (ex: -sf) in argument parsing so that it's more compatible with previous implementation.

Just know that i did not take the time to test that CI/CD related stuff "still works". I think "they still work", but because of the "simple" nature of the changes of this PR i'm too lazy to go out of my way to test that forking the repo and following the instructions still work. I'm just saying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants