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

ASCIIcast #19

Merged
merged 10 commits into from
Mar 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ close to running containers. [krunvm] creates and starts VMs based on the
multi-platform OCI images created for this project -- [ubuntu] (default) or
[fedora].

![Demo](./demo/demo.gif)

[self]: https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners
[runners]: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners
[krunvm]: https://github.com/containers/krunvm
Comment on lines 8 to 15
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [5-5]

There's a minor typo in "infrastruture". It should be corrected to "infrastructure" for clarity.

- inside your infrastruture, as opposed to
+ inside your infrastructure, as opposed to

Expand Down
26 changes: 26 additions & 0 deletions demo/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Demo with terminalizer

## Recording

Provided you have [terminalizer] installed, run the following from the top
directory of this repository.
Comment on lines +5 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the instructions for installing Terminalizer are clear and accessible for users who may not have it installed. Consider adding a sentence or link to Terminalizer's installation guide.

Would you like me to add a brief installation guide or a link to Terminalizer's official installation documentation?


```bash
terminalizer record demo/demo.yml --config demo/config.yml
```

## Verifying

Once done, use the [`play`][play] sub-command to verify your recording. You
might want to remove output lines from the YAML, alternatively change the pace.

## Render

Finally, run the following command to generate the animated GIF
Copy link
Contributor

Choose a reason for hiding this comment

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

There's an extra whitespace before the code block. While this is a minor formatting issue, maintaining consistency in the document's formatting can improve readability.

- Finally, run the following command to generate the animated GIF  
+ Finally, run the following command to generate the animated GIF

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
Finally, run the following command to generate the animated GIF
Finally, run the following command to generate the animated GIF


```bash
terminalizer render demo/demo.yml --output demo/demo.gif
```

[terminalizer]: https://github.com/faressoft/terminalizer
[play]: https://github.com/faressoft/terminalizer?tab=readme-ov-file#play
107 changes: 107 additions & 0 deletions demo/config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Specify a command to be executed
# like `/bin/bash -l`, `ls`, or any other commands
# the default is bash for Linux
# or powershell.exe for Windows
command: ./demo/demo.sh

# Specify the current working directory path
# the default is the current working directory path
cwd: null

# Export additional ENV variables
env:
recording: true

# Explicitly set the number of columns
# or use `auto` to take the current
# number of columns of your shell
cols: 180

# Explicitly set the number of rows
# or use `auto` to take the current
# number of rows of your shell
rows: 40

# Amount of times to repeat GIF
# If value is -1, play once
# If value is 0, loop indefinitely
# If value is a positive number, loop n times
repeat: 0

# Quality
# 1 - 100
quality: 95

# Delay between frames in ms
# If the value is `auto` use the actual recording delays
frameDelay: auto

# Maximum delay between frames in ms
# Ignored if the `frameDelay` isn't set to `auto`
# Set to `auto` to prevent limiting the max idle time
maxIdleTime: auto

# The surrounding frame box
# The `type` can be null, window, floating, or solid`
# To hide the title use the value null
# Don't forget to add a backgroundColor style with a null as type
frameBox:
type: floating
title: GitHub Runner in KrunVM
style:
border: 0px black solid
# boxShadow: none
# margin: 0px

# Add a watermark image to the rendered gif
# You need to specify an absolute path for
# the image on your machine or a URL, and you can also
# add your own CSS styles
watermark:
imagePath: null
style:
position: absolute
right: 15px
bottom: 15px
width: 100px
opacity: 0.9

# Cursor style can be one of
# `block`, `underline`, or `bar`
cursorStyle: block

# Font family
# You can use any font that is installed on your machine
# in CSS-like syntax
fontFamily: "Monaco, Lucida Console, Ubuntu Mono, Monospace"

# The size of the font
fontSize: 12

# The height of lines
lineHeight: 1

# The spacing between letters
letterSpacing: 0

# Theme
theme:
background: "transparent"
foreground: "#afafaf"
cursor: "#c7c7c7"
black: "#232628"
red: "#fc4384"
green: "#b3e33b"
yellow: "#ffa727"
blue: "#75dff2"
magenta: "#ae89fe"
cyan: "#708387"
white: "#d5d5d0"
brightBlack: "#626566"
brightRed: "#ff7fac"
brightGreen: "#c8ed71"
brightYellow: "#ebdf86"
brightBlue: "#75dff2"
brightMagenta: "#ae89fe"
brightCyan: "#b1c6ca"
brightWhite: "#f9f9f4"
29 changes: 17 additions & 12 deletions demo/demo-magic/demo-magic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,13 @@ function p() {
fi

# render the prompt
x=$(PS1="$DEMO_PROMPT" "$BASH" --norc -i </dev/null 2>&1 | sed -n '${s/^\(.*\)exit$/\1/p;}')
x=$(PS1="$DEMO_PROMPT" "$BASH" --norc -i </dev/null 2>&1 | sed -n 's/^\(.*\)exit$/\1/p;')

# show command number is selected
if $SHOW_CMD_NUMS; then
printf "[$((++C_NUM))] $x"
printf "[$((++C_NUM))] %s" "$x"
else
printf "$x"
printf "%s" "$x"
fi

# wait for the user to press a key before typing the command
Expand All @@ -105,7 +105,7 @@ function p() {
if [[ -z $TYPE_SPEED ]]; then
echo -en "$cmd"
else
echo -en "$cmd" | pv -qL $[$TYPE_SPEED+(-2 + RANDOM%5)];
echo -en "$cmd" | pv -qL $((TYPE_SPEED+(-2 + RANDOM%5)));
fi

# wait for the user to press a key before moving on
Expand Down Expand Up @@ -151,20 +151,20 @@ function pei {
##
function cmd() {
# render the prompt
x=$(PS1="$DEMO_PROMPT" "$BASH" --norc -i </dev/null 2>&1 | sed -n '${s/^\(.*\)exit$/\1/p;}')
printf "$x\033[0m"
read command
x=$(PS1="$DEMO_PROMPT" "$BASH" --norc -i </dev/null 2>&1 | sed -n 's/^\(.*\)exit$/\1/p;')
printf "%s\033[0m" "$x"
read -r command
run_cmd "${command}"
}

function run_cmd() {
function handle_cancel() {
printf ""
}
function handle_cancel() {
printf ""
}

function run_cmd() {
trap handle_cancel SIGINT
stty -echoctl
eval $@
eval "$*"
stty echoctl
trap - SIGINT
}
Expand Down Expand Up @@ -216,6 +216,11 @@ while getopts ":dhncw:" opt; do
w)
PROMPT_TIMEOUT=$OPTARG
;;
*)
echo "Invalid option: -$OPTARG" >&2
usage
exit 1
;;
Comment on lines +219 to +223
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The addition of error handling for invalid options in the script is a good practice, and it enhances the user experience by providing clear feedback on incorrect usage. However, the usage instructions are not fully comprehensive, as they do not include all the options handled by the script, specifically the '-c' option. Ensure that all potential options are covered in the usage instructions for completeness.

Analysis chain

The addition of error handling for invalid options in the script is a good practice. It enhances the user experience by providing clear feedback on incorrect usage. Ensure that all potential options are covered and that the usage instructions are comprehensive.



Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that all options are covered in the error handling and usage instructions.
grep -C3 "Invalid option" demo/demo-magic/demo-magic.sh | grep "case"

Length of output: 69



Script:

#!/bin/bash
# Extract the entire case statement that handles options in the script.
sed -n '/case/,/esac/p' demo/demo-magic/demo-magic.sh

Length of output: 473



Script:

#!/bin/bash
# Extract the implementation of the usage function to verify comprehensive instructions.
grep -A10 "usage() {" demo/demo-magic/demo-magic.sh

Length of output: 423

esac
done

Expand Down
Binary file added demo/demo.gif
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 6 additions & 3 deletions demo/demo.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ fi

clear

pe "# For the sake of the asciicast: PAT is present in RUNNER_PAT environment variable"
pe "# -r 1: to run once only, the default is to run forever"
pe "./orchestrator.sh -v -- -r 1 -p efrecon/gh-runner-krunvm"
pei "# Let's start one (artificially short-lived) runner for this repository"
pei "# A PAT is present in the environment variable RUNNER_PAT"
pei "# Just for the demo, we will use two seldom used options to show teardown"
pei "# -k 40: to run for 40 seconds only, the default is to run forever, until a job is picked"
pei "# -r 1: to run once only, the default is to create ephemeral runners forever"
pei "./orchestrator.sh -v -- -r 1 -k 40 -p efrecon/gh-runner-krunvm"
21 changes: 16 additions & 5 deletions runner.sh
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,14 @@ RUNNER_REPEAT=${RUNNER_REPEAT:-"-1"}
# Secret to be used to request for loop end. Good default is a random string.
RUNNER_SECRET=${RUNNER_SECRET:-"$(random_string)"}

# Number of seconds after which to terminate (empty for never, the good default)
RUNNER_TERMINATE=${RUNNER_TERMINATE:-""}
Comment on lines +115 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The search results confirm that RUNNER_TERMINATE is utilized within runner.sh for managing the runner's termination after a specified duration. However, there is no evidence of its documentation in a user-accessible form, such as a help or usage section. This suggests that the review comment about ensuring the new variable is documented for users remains valid, as there's no indication that such documentation has been provided.

Analysis chain

The introduction of RUNNER_TERMINATE to specify a termination time for the runner is a useful feature for managing ephemeral runners. Ensure that this new variable is documented in the script's usage instructions or help output to inform users of its availability.


Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify if the RUNNER_TERMINATE option is documented in the script's usage or help section.
grep -C3 "Usage:" runner.sh | grep "RUNNER_TERMINATE"

Length of output: 51



Script:

#!/bin/bash
# Broaden the search for any mention of RUNNER_TERMINATE within runner.sh to verify its documentation.
grep "RUNNER_TERMINATE" runner.sh

Length of output: 252


# shellcheck disable=SC2034 # Used in sourced scripts
KRUNVM_RUNNER_DESCR="Create runners forever using krunvm"


while getopts "c:d:D:g:G:i:l:L:m:M:p:r:s:S:T:u:Uvh-" opt; do
while getopts "c:d:D:g:G:i:l:L:m:M:p:k:r:s:S:T:u:Uvh-" opt; do
case "$opt" in
c) # Number of CPUs to allocate to the VM
RUNNER_CPUS="$OPTARG";;
Expand Down Expand Up @@ -144,6 +147,8 @@ while getopts "c:d:D:g:G:i:l:L:m:M:p:r:s:S:T:u:Uvh-" opt; do
fi;;
p) # Principal to authorise the runner for, name of repo, org or enterprise
RUNNER_PRINCIPAL="$OPTARG";;
k) # Kill and terminate after this many seconds
RUNNER_TERMINATE="$OPTARG";;
r) # Number of times to repeat the runner loop
RUNNER_REPEAT="$OPTARG";;
s) # Scope of the runner, one of repo, org or enterprise
Expand Down Expand Up @@ -268,6 +273,10 @@ EOF
RUNNER_PID=$!
eval "$optstate"; # Restore options
verbose "Started microVM '${RUNNER_PREFIX}-$_id' with PID $RUNNER_PID"
if [ -n "$RUNNER_TERMINATE" ]; then
verbose "Terminating runner in $RUNNER_TERMINATE seconds"
sleep "$RUNNER_TERMINATE" && cleanup &
fi
wait "$RUNNER_PID"
RUNNER_PID=
}
Expand All @@ -280,8 +289,10 @@ vm_delete() {
warning "Removing isolation environment ${RUNNER_ENVIRONMENT}/${1}.env"
rm -f "${RUNNER_ENVIRONMENT}/${1}.env"
fi
verbose "Removing microVM '${RUNNER_PREFIX}-$1'"
run_krunvm delete "${RUNNER_PREFIX}-$1"
if run_krunvm list | grep -qE "^${RUNNER_PREFIX}-$1"; then
verbose "Removing microVM '${RUNNER_PREFIX}-$1'"
run_krunvm delete "${RUNNER_PREFIX}-$1"
fi
}

vm_terminate() {
Expand Down Expand Up @@ -313,10 +324,10 @@ vm_terminate() {

cleanup() {
trap '' EXIT
if [ -n "$RUNNER_PID" ]; then
if [ -n "${RUNNER_PID:-}" ]; then
vm_terminate
fi
if [ -n "$RUNNER_ID" ]; then
if [ -n "${RUNNER_ID:-}" ]; then
vm_delete "$RUNNER_ID"
fi
}
Expand Down
Loading