README.md (2)

Line range hint 75-79: LGTM with a suggestion: Clear feature description with community engagement

The new feature description accurately reflects the added support for both krunvm and krun under podman, aligning with the PR objectives. The mention of potential compatibility with container-based solutions is a good way to encourage community involvement.

Consider adding a link to the project's issue tracker or contribution guidelines after the sentence "Reports and/or changes are welcome." This would make it easier for interested contributors to get involved.

---

98-102: LGTM with a minor formatting suggestion: Clear explanation of runtime requirements

The updated requirements section accurately reflects the new runtime options introduced in this PR. The note about podman not being required when using krunvm is a helpful clarification for users.

There's a minor formatting inconsistency. Consider removing the extra space before the krun bullet point to align it with the krunvm bullet point:

 + A compatible runtime, i.e. either:
-  + `krun` and [`podman`][podman].
+ + `krun` and [`podman`][podman].
   + `krunvm`, its [requirements] and `buildah`

🧰 Tools

🪛 LanguageTool

[uncategorized] ~100-~100: Loose punctuation mark.
Context: ...un and [podman][podman]. + krunvm, its [requirements] and buildah` Note:...

(UNLIKELY_OPENING_PUNCTUATION)

lib/microvm.sh (3)

115-115: Use consistent DNS default value

The default DNS server is set to 1.1.1.1. Consider using a more widely recognized default DNS server like 8.8.8.8, or making it configurable to suit different environments.

---

183-184: Fix comment regarding job control

The comment states Disable job control, but set -m actually enables job control (monitor mode). Update the comment to reflect the correct behavior.

Correct the comment:

       optstate=$(set +o)
-      set -m; # Disable job control
+      set -m; # Enable job control

---

235-235: Typo in comment

There's a typo in the comment: "Specify howlog to wait between TERM and KILL?" It should be "Specify how long to wait between TERM and KILL?"

Correct the comment:

     # TODO: Specify howlog to wait between TERM and KILL?
+    # TODO: Specify how long to wait between TERM and KILL?

runner.sh (3)

Line range hint 215-289: Potential issue with argument handling in vm_run function

The vm_run function uses set -- multiple times, which can overwrite previously set positional parameters and lead to unexpected behavior.

Consider using an array to collect arguments to prevent overwriting. Here's a suggested refactor:

+args=()
...
  if [ -n "$RUNNER_ENVIRONMENT" ]; then
    ...
    # Pass the location of the env. file to the runner script
-   set -- -E "/${RUNNER_VM_ENVDIR}/${_id}.env"
-   set -- -k "/${RUNNER_VM_ENVDIR}/${_id}.tkn" "$@"
+   args+=("-E" "/${RUNNER_VM_ENVDIR}/${_id}.env")
+   args+=("-k" "/${RUNNER_VM_ENVDIR}/${_id}.tkn")
  else
    ...
    # Collect arguments
+   args+=(
+     -g "$RUNNER_GITHUB"
+     -G "$RUNNER_GROUP"
+     -i "$_id"
+     -l "$RUNNER_LOG"
+     -L "$RUNNER_LABELS"
+     -p "$RUNNER_PRINCIPAL"
+     -s "$RUNNER_SCOPE"
+     -S "$RUNNER_SECRET"
+     -T "$RUNNER_PAT"
+     -u "$RUNNER_USER"
+   )
    if is_true "$RUNNER_EPHEMERAL"; then
-     set -- -e "$@"
+     args+=("-e")
    fi
    for _ in $(seq 1 "$RUNNER_VERBOSE"); do
-     set -- -v "$@"
+     args+=("-v")
    done
  fi

  # Add image to create from
- set -- -- "$RUNNER_IMAGE" "$@"
+ args=( "--" "$RUNNER_IMAGE" "${args[@]}" )

  # Add microVM options
- set -- \
-     -c "$RUNNER_CPUS" \
-     -m "$RUNNER_MEMORY" \
-     -d "$RUNNER_DNS" \
-     -n "${RUNNER_PREFIX}-$_id" \
-     -e "$RUNNER_ENTRYPOINT" \
-     "$@"
+ args=(
+   -c "$RUNNER_CPUS"
+   -m "$RUNNER_MEMORY"
+   -d "$RUNNER_DNS"
+   -n "${RUNNER_PREFIX}-$_id"
+   -e "$RUNNER_ENTRYPOINT"
+   "${args[@]}"
+ )

  if [ -n "${RUNNER_DIR:-}" ]; then
-   set -- -v "${RUNNER_ROOTDIR}:${RUNNER_DIR}" "$@"
+   args+=("-v" "${RUNNER_ROOTDIR}:${RUNNER_DIR}")
  fi
  if [ -n "${RUNNER_ENVIRONMENT:-}" ]; then
-   set -- -v "${RUNNER_ENVIRONMENT}:/${RUNNER_VM_ENVDIR}" "$@"
+   args+=("-v" "${RUNNER_ENVIRONMENT}:/${RUNNER_VM_ENVDIR}")
  fi
  if [ -n "$RUNNER_MOUNT" ]; then
    while IFS= read -r mount || [ -n "$mount" ]; do
      if [ -n "$mount" ]; then
-       set -- -v "$mount" "$@"
+       args+=("-v" "$mount")
      fi
    done <<EOF
$(printf %s\\n "$RUNNER_MOUNT")
EOF
  fi

  trace "Running microVM with: ${args[*]}"
  microvm_run "${args[@]}"

---

278-286: Simplify mounting multiple directories

The loop handling RUNNER_MOUNT can be simplified for clarity and efficiency.

Consider using a for loop:

- while IFS= read -r mount || [ -n "$mount" ]; do
-   if [ -n "$mount" ]; then
-     set -- -v "$mount" "$@"
-   fi
- done <<EOF
- $(printf %s\\n "$RUNNER_MOUNT")
- EOF
+ for mount in $RUNNER_MOUNT; do
+   [ -n "$mount" ] && args+=("-v" "$mount")
+ done

---

Line range hint 42-45: Duplicate definitions of RUNNER_DIR and RUNNER_MOUNT

The variables RUNNER_DIR and RUNNER_MOUNT are defined twice in the script, which may cause confusion and unexpected behavior.

Remove the duplicate definitions to maintain clarity:

-# Name of top directory in VM where to host a copy of the root directory of this
-# script. When this is set, the runner starter script from that directory will
-# be used -- instead of the one already in the OCI image. This option is mainly
-# usefull for development and testing.
-RUNNER_DIR=${RUNNER_DIR:-""}
-
-RUNNER_MOUNT=${RUNNER_MOUNT:-""}

Also applies to: 83-86

Configuration used: CodeRabbit UI
Review profile: CHILL