chore: Rework setup script #27

halostatue · 2024-11-22T03:10:21Z

halostatue · 2024-11-22T03:13:41Z

I’m going to heavily annotate the changes I made to explain why. It makes a lot of changes that may or may not work with your ultimate goals, but inasmuch as I can test it locally (when I have everything except PostgreSQL installed), it seems to work once I added a DRY_RUN flag.

$ mkdir z && touch z/{.bashrc,.zshrc}
$ DRY_RUN=1 HOME=z priv/scirpt.sh

halostatue · 2024-11-22T03:15:02Z

priv/script.sh

@@ -2,441 +2,490 @@

 set -eu

+elixir_version=1.17.3-otp-27


I moved these to the top of the script because they are the things most likely to be updated frequently. I have also updated them to the latest versions for Elixir and OTP.

halostatue · 2024-11-22T03:16:05Z

priv/script.sh

+phoenix_version=1.7.14
+postgres_version=15.1
+
+OS="$(uname -s)"


This has been partially moved upwards to improve $HOME resolution. See below.

halostatue · 2024-11-22T03:17:49Z

priv/script.sh

+  ;;
+esac
+
+has() {


This simplifies reading some tests. I also modified a couple of cases where mise which was used to match the versions installed. This can be simplified if you just want to use any installed version of Elixir and/or OTP.

halostatue · 2024-11-22T03:19:04Z

priv/script.sh

+  if [ "$os_type" = Linux ]; then
+    HOME="$(getent passwd "$USER" 2>/dev/null | cut -d: -f6)"
+  elif [ "$os_type" = macOS ]; then
+    HOME="$(dscl . -read /Users/"$USER" NFSHomeDirectory)"


This is how you read the home directory on macOS. It means that the fallback on line 53 is less necessary in general.

halostatue · 2024-11-22T03:19:35Z

priv/script.sh

+phx_tools_path="$HOME"/.config/phx_tools
+phx_tools_sh_path="$phx_tools_path"/phx_tools.sh
+phx_tools_fish_path="$phx_tools_path"/phx_tools.fish
+homebrew_path=


These simplify processing later.

halostatue · 2024-11-22T03:22:58Z

priv/script.sh

+
+# Disable colour output if NO_COLOR is set to any value at all.
+# https://no-color.org
+if [ -n "${NO_COLOR:-}" ]; then


$NO_COLOR should be respected, and tput should be preferred over explicit ANSI escapes.

halostatue · 2024-11-22T03:28:44Z

priv/script.sh

-        printf "Invalid name argument on checking: %s\n" "$1"
-        exit 1
-        ;;
+install_deps() {


install_deps() was added because it simplifies the reading and reasoning about the various steps.

halostatue · 2024-11-22T03:30:07Z

priv/script.sh

+    dnf) sudo dnf install -y unzip ;;
+    pacman) sudo pacman -Sy --noconfirm unzip ;;
+    apk) sudo apk add --no-cache unzip ;;
+    esac


If set -e requires case "completeness" to prevent an error halt, then add *) : ;; to the end of the case block and it will swallow unknown values.

halostatue · 2024-11-22T03:33:12Z

priv/script.sh

-        printf "%s is already installed. Skipping...\n" "$1"
-    else
-        printf "Installing %s...\n" "$1"
+install_shell_scripts() {


This is the real change here. Instead of modifying just ~/.bashrc, ~/.zshrc, or ~/.config/fish/config.fish, we're creating ~/.config/phx_tools/phx_tools.{sh,fish} with the appropriate configuration changes.

The sh version is written to work with both Bash and zsh.

halostatue · 2024-11-22T03:34:38Z

priv/script.sh

-        install "$1"
+  sh_source_line="source '$phx_tools_sh_path'"
+
+  if has bash && [ -f "$HOME"/.bashrc ]; then


Here we update $HOME/.bashrc to source ~/.config/phx_tools/phx_tools.sh, but only if bash is installed and ~/.bashrc exists.

halostatue · 2024-11-22T03:37:07Z

priv/script.sh


-sleep 3
+printf "%s\n" "$bblue$banner"


All of the print statements have been condensed to reduce the whitespace. It was scrolling too much on my 56-line shell.

almirsarajcic · 2024-11-22T06:43:40Z

Wow, the best PR I've seen in my life!
Thanks for all the annotations. My shell scripting is bad, so this is a huge learning opportunity for me.

Most of the things you've changed I agree with. The rest I don't understand yet.

I'm only worried about whether we'll still be able to run the GitHub Actions workflow with these changes.
We don't have much in the expect script, so I thought our tests would be robust enough.

phx.tools/test/scripts/script.exp

Lines 1 to 9 in a24a101

    
           #!/usr/bin/expect 
        
           set script [lindex $argv 0] 
        
           set timeout -1 
        
           spawn ./../../priv/script.sh 
        
           match_max 100000 
        
           expect "Do you want to continue? (y/n) " 
        
           send -- "y\r" 
        
           expect eof

But they failed at line

spawn ./../../priv/script.sh
tput: No value for $TERM and no -T specified

halostatue · 2024-11-22T07:51:18Z

Wow, the best PR I've seen in my life! Thanks for all the annotations. My shell scripting is bad, so this is a huge learning opportunity for me.

Thanks for the kind words. I’ve been shell scripting for decades now, so this really wasn't too hard.

Most of the things you've changed I agree with. The rest I don't understand yet.

I'm only worried about whether we'll still be able to run the GitHub Actions workflow with these changes. We don't have much in the expect script, so I thought our tests would be robust enough.

I didn't look at this from the perspective of testing, but from user experience. The test failures did point out a bug, and I should have tested for it, but it's sort of hard to test for on a machine. I’ve put a fixup commit that should resolve the test failures in the expect script. It makes a couple of changes to the expect script to make sure that a bit more is seen than was previously expected.

almirsarajcic · 2024-11-22T09:21:13Z

spawn ./../../priv/script.sh
Unsupported shell: bash
expect: spawn id exp3 not open
    while executing
"expect "phx.tools setup is complete!""
    (file "script.exp" line 13)

This time I think it's because of this line
https://github.com/optimumBA/phx.tools/pull/27/files#diff-5cd7660b84bc8ab514a6b77b04c735c4581ddde80ed19035e9f08ded4a34aa50R83

*/bash | */fish | */zsh) : ;;

phx.tools/.github/workflows/pr.yml

Lines 289 to 292 in a24a101

    
           - name: Test the script 
        
             if: steps.result_cache.outputs.cache-hit != 'true' 
        
             run: cd test/scripts && expect script.exp 
        
             shell: bash -l {0}

https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#defaultsrunshell
Not sure if /bin/ prefix works for the shell option.

halostatue · 2024-11-22T14:34:43Z

I'll look at that a bit later. Probably by pushing a debugging fix.

- Add dependency checking with dependabot. - Put more specific version locks for actions. - Upgrade actions which were using Node 16 actions.

halostatue · 2024-11-23T01:25:07Z

.github/dependabot.yml

@@ -0,0 +1,11 @@
+version: 2


I added this because when I tested the workflows locally, I got warnings about Node 16 workflows. You can remove if you like.

halostatue · 2024-11-23T01:30:26Z

.github/github_workflows.ex

-          uses: "superfly/flyctl-actions/setup-flyctl@master"
+          uses: "superfly/flyctl-actions/[email protected]",
+          with: [
+            version: "0.3.40"


The setup-flyctl action recommends pinning the flyctl version for production releases, so I did so in addition to pinning the action version itself. This will need monitoring separately for upgrades.

halostatue · 2024-11-23T01:31:40Z

.github/github_workflows.ex

@@ -227,7 +230,7 @@ defmodule GithubWorkflows do
      steps: [
        [
          name: "Restore PLT cache",
-          uses: "actions/cache@v3",
+          uses: "actions/cache@v4.1.2",


v3 uses node 16, v4 uses node 20.

halostatue · 2024-11-23T01:32:01Z

.github/github_workflows.ex

@@ -455,7 +458,7 @@ defmodule GithubWorkflows do
            [
              name: "Check HTTP status code",
              if: "steps.result_cache.outputs.cache-hit != 'true'",
-              uses: "nick-fields/retry@v2",
+              uses: "nick-fields/retry@v3.0.0",


v2 uses node 16; v3 uses node 20.

halostatue commented Nov 22, 2024

View reviewed changes

halostatue force-pushed the reworked-setup-and-config branch from e76cd1b to 09316ee Compare November 22, 2024 03:22

halostatue commented Nov 22, 2024

View reviewed changes

halostatue force-pushed the reworked-setup-and-config branch 3 times, most recently from 4c0cf37 to 6b583d5 Compare November 23, 2024 00:44

halostatue added 2 commits November 22, 2024 20:19

chore: Rework setup script

88f6a29

chore: Improve workflow security

9319c8e

- Add dependency checking with dependabot. - Put more specific version locks for actions. - Upgrade actions which were using Node 16 actions.

halostatue force-pushed the reworked-setup-and-config branch from bd54cb5 to 9319c8e Compare November 23, 2024 01:21

halostatue commented Nov 23, 2024

View reviewed changes

halostatue had a problem deploying to pr-27 November 23, 2024 10:03 — with GitHub Actions Failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Rework setup script #27

chore: Rework setup script #27

halostatue commented Nov 22, 2024 •

edited

Loading

halostatue commented Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

halostatue Nov 22, 2024

almirsarajcic commented Nov 22, 2024

halostatue commented Nov 22, 2024

almirsarajcic commented Nov 22, 2024

halostatue commented Nov 22, 2024

halostatue Nov 23, 2024

halostatue Nov 23, 2024

halostatue Nov 23, 2024

halostatue Nov 23, 2024

+                ;;
+              esac
+              has() {

chore: Rework setup script #27

Are you sure you want to change the base?

chore: Rework setup script #27

Conversation

halostatue commented Nov 22, 2024 • edited Loading

halostatue commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almirsarajcic commented Nov 22, 2024

halostatue commented Nov 22, 2024

almirsarajcic commented Nov 22, 2024

halostatue commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halostatue commented Nov 22, 2024 •

edited

Loading