feat: Add yq to act and custom #117

Beanow · 2023-11-03T13:29:52Z

Resolves #78

I gave this a simple test build locally (linux x86_64).

docker build -f linux/ubuntu/Dockerfile \
--build-arg FROM_IMAGE=buildpack-deps \
--build-arg FROM_TAG=22.04 \
--build-arg TYPE=act \
-t test-act \
.

#         🧨 Executing yq.sh 🧨
# + /imagegeneration/installers/yq.sh
# Downloading 'https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64' to '/tmp/yq'...
# Download completed
# Performing checksum verification
# Checksum verification passed

docker run --rm test-act yq --version
# yq (https://github.com/mikefarah/yq/) version v4.35.2

Since there's a lot of hard-coded references to this path.

Unlike upstream we detect the arch suffix here.

Beanow · 2023-11-03T13:48:17Z

11 minutes of pulling later... can confirm it's the same as:

docker run --rm catthehacker/ubuntu:full-latest yq --version
# yq (https://github.com/mikefarah/yq/) version v4.35.2

catthehacker · 2023-11-07T21:04:06Z

linux/ubuntu/scripts/act.sh

+case "$(uname -m)" in
+  'aarch64')
+    scripts=(
+      yq
+    )
+    ;;
+  'x86_64')
+    scripts=(
+      yq
+    )
+    ;;
+  *) exit 1 ;;
+esac
+
+for SCRIPT in "${scripts[@]}"; do
+  printf "\n\t🧨 Executing %s.sh 🧨\t\n" "${SCRIPT}"
+  "/imagegeneration/installers/${SCRIPT}.sh"
+done


this breaks building on any arch other than aarch64/x86_64

Especially a problem, because the act base image is built for armhf, aarch64 and x86_64.

The flavour images are not built for armhf and usually don't work on any arch other than aarch64 (rust image not built due to cpu intensive compiling in qemu)/x86_64.

Yeah good point, I looked at the wrong matrix. The yq script already supports arm/v7.
I'll have a look at updating the cases.

Does this approach otherwise look OK?

Added the missing arch

ChristopherHX

Some thoughts from my side...

I would be careful if you remove double quotes in bash, have bad experience in the past.

ChristopherHX · 2023-11-19T13:36:33Z

linux/ubuntu/scripts/helpers/install.sh

+    else
+        local ERR_EXIT_ENABLED=false
+    fi
+    set +e


Why did you add ERR_EXIT_ENABLED if it is ignored in the first iteration due to unconditional set +e?

The changes to this script aren't my own. They're from the upstream repository.

Primary reason I found the sync was necessary was because checksum validation code was added and used for the yq download.

I'll have to defer to the blame upstream for your questions though :]

Here it's actions/runner-images#8352

Just to be clear, I'm not defending the decisions upstream. I don't have an opinion on the suggestions you made here.

But I think there's maintenance tradeoffs between how closely you want to track the other repo. So I feel the decision to diverge and track our own patches here should be up to the maintainers and not me :]

Feel free to edit, or request changes.

ChristopherHX · 2023-11-19T13:36:58Z

linux/ubuntu/scripts/helpers/install.sh

@@ -49,21 +57,156 @@ download_with_retries() {
 ##     echo "packageName is not installed!"
 ## fi
 IsPackageInstalled() {
-  dpkg -S "$1" &>/dev/null
+    dpkg -S $1 &> /dev/null


Is this necessary to remove quotes?

Removing quotes in bash is dangerous.

Example:

listargs() { while [[ -n "$1" ]]; do echo $1; shift; done }; IsPackageInstalled() { listargs -S $1; } ; IsPackageInstalled "hello or";

argument hello or ends up beeing interpreted by bash.

Seems upstream this was never quoted.

I found this, but is just a move. The code before was initial commit.
actions/runner-images@a7ee8ab

ChristopherHX · 2023-11-19T13:41:08Z

linux/ubuntu/scripts/custom.sh

@@ -41,9 +41,10 @@ case "$(uname -m)" in
      go
      js
      dotnet
+      yq


Why do you want to install yq twice in the custom image?

act-latest is the base of custom-latest, js-latest, go-latest and so on

Good catch. That's just me being unfamiliar in the codebase 😂

ChristopherHX · 2023-11-19T14:08:27Z

linux/ubuntu/scripts/yq.sh

+external_hash=$(get_hash_from_remote_file "${base_url}/checksums" "${filename} " "" " " "19")
+use_checksum_comparison "/tmp/yq" "${external_hash}"
+# Install YQ
+sudo install /tmp/yq /usr/bin/yq


Maybe /usr/local/bin/yq to avoid conflicts with apt?

Likewise this script I tried to make minimal changes to compared to upstream.

I'm introducing different architectures here compared to upstream, which only support amd64.

It seems to be a deliberate workflow compatibility choice to install it here though.
actions/runner-images#3768

ChristopherHX · 2023-11-19T14:23:01Z

linux/ubuntu/scripts/helpers/install.sh

+        http_code=$(eval $COMMAND)
+        exit_code=$?
+        test "$ERR_EXIT_ENABLED" = true && set -e
+        if [ $http_code -eq 200 ] && [ $exit_code -eq 0 ]; then


Why did you remove quotes for http_code? We don't control the output of curl, it may be able to inject stuff.

For example exit_code can only be a number, because that comes from bash itself.

See upstream actions/runner-images#3721

ChristopherHX

All fine now. I'm not familar with the pre 2023 history of this repository, there was a linter requiring changes from upstream files.

Beanow added 4 commits November 3, 2023 14:19

feat: update install script

0c7bc59

fix: revert toolset path

fcbf232

Since there's a lot of hard-coded references to this path.

feat: Adds yq script

f1d6639

Unlike upstream we detect the arch suffix here.

feat: include yq in act and custom flavors

5cbbf73

catthehacker reviewed Nov 7, 2023

View reviewed changes

fix: include armhf in supported arch for act.yml

ede115c

Beanow requested review from ChristopherHX and catthehacker November 13, 2023 19:44

ChristopherHX reviewed Nov 19, 2023

View reviewed changes

fix: PR feedback, don't need to install twice for custom

8d55b05

ChristopherHX approved these changes Nov 19, 2023

View reviewed changes

ChristopherHX merged commit ec09e6e into catthehacker:master Nov 19, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add yq to act and custom #117

feat: Add yq to act and custom #117

Beanow commented Nov 3, 2023

Beanow commented Nov 3, 2023

catthehacker Nov 7, 2023

ChristopherHX Nov 10, 2023

Beanow Nov 13, 2023

Beanow Nov 13, 2023

ChristopherHX left a comment

ChristopherHX Nov 19, 2023

Beanow Nov 19, 2023

Beanow Nov 19, 2023 •

edited

Loading

ChristopherHX Nov 19, 2023

Beanow Nov 19, 2023

ChristopherHX Nov 19, 2023

Beanow Nov 19, 2023

ChristopherHX Nov 19, 2023

Beanow Nov 19, 2023 •

edited

Loading

ChristopherHX Nov 19, 2023

Beanow Nov 19, 2023

ChristopherHX left a comment

@@ @@ -41,9 +41,10 @@ case "$(uname -m)" in @@
                     go
                     js
                     dotnet
+                    yq

feat: Add yq to act and custom #117

feat: Add yq to act and custom #117

Conversation

Beanow commented Nov 3, 2023

Beanow commented Nov 3, 2023

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

ChristopherHX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beanow Nov 19, 2023 • edited Loading

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

Beanow Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherHX left a comment

Choose a reason for hiding this comment

Beanow Nov 19, 2023 •

edited

Loading

Beanow Nov 19, 2023 •

edited

Loading