Add demo workflow #872
Add demo workflow #872
Conversation
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
📝 WalkthroughWalkthroughThis pull request introduces a new GitHub Actions workflow that automates the preparation, building, and updating of Atmos demo assets while removing an older VHS-specific workflow. It updates build scripts for cross-platform compatibility, revises configuration files for VHS and Terraform modules, and adds numerous tape files to simulate Atmos CLI interactions. A new Go-based CLI tool with TUI functionality for processing recordings is also introduced, alongside dependency and alias updates. Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/demo.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
20-20: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/demo.yml (3)
1-11: LGTM! Environment setup is solid, warrior!
The manual trigger and environment variables are well-configured for demo generation.
82-91: Well-configured concurrency control, champion!
The matrix strategy with concurrency control is well-implemented to handle multiple .tape files efficiently.
55-58: Verify the existence of the Makefile, brave one!
The build step assumes the existence of demo/screengrabs/Makefile.
✅ Verification successful
Victory! The Makefile and its dependencies stand ready for battle!
The build system is well-structured and complete:
- Makefile exists with both required targets:
build-allandinstall build-all.shscript is present and functional- Required input file
demo-stacks.txtis confirmed - Install path is properly configured to
../../website/src/components/screengrabs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Makefile exists and contains required targets
if [ -f demo/screengrabs/Makefile ]; then
echo "Makefile found. Checking for required targets..."
grep -E "^(build-all|install):" demo/screengrabs/Makefile
else
echo "Makefile not found at demo/screengrabs/Makefile"
exit 1
fi
Length of output: 226
Script:
#!/bin/bash
# Let's check the content of the Makefile to understand the targets
echo "=== Content of demo/screengrabs/Makefile ==="
cat demo/screengrabs/Makefile
Length of output: 419
Script:
#!/bin/bash
# Let's verify the build script exists and check its content
echo "=== Checking build-all.sh existence and content ==="
if [ -f demo/screengrabs/build-all.sh ]; then
echo "build-all.sh found. Content:"
cat demo/screengrabs/build-all.sh
echo -e "\n=== Checking demo-stacks.txt existence ==="
if [ -f demo/screengrabs/demo-stacks.txt ]; then
echo "demo-stacks.txt found"
else
echo "demo-stacks.txt not found"
fi
else
echo "build-all.sh not found"
fi
Length of output: 2581
.github/workflows/demo.yml
Outdated
| - name: Push changes to branch | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| JOB_NAME=$(basename "${{ matrix.file }}" | sed 's/.tape$//') | ||
| BRANCH_NAME="chore/update-$JOB_NAME-for-$VERSION" | ||
| git checkout -b "$BRANCH_NAME" || git checkout "$BRANCH_NAME" | ||
| git add -A | ||
| git commit -m "chore: update $JOB_NAME for $VERSION" || echo "No changes to commit" | ||
| git push origin "$BRANCH_NAME" | ||
|
|
||
| - name: Create or update PR | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| JOB_NAME=$(basename "${{ matrix.file }}" | sed 's/.tape$//') | ||
| BRANCH_NAME="chore/update-$JOB_NAME-for-$VERSION" | ||
| PR_TITLE="Update $JOB_NAME for $VERSION" | ||
| PR_BODY="This PR updates the demo gif for $JOB_NAME with Atmos version $VERSION." | ||
| gh pr view "$BRANCH_NAME" > /dev/null 2>&1 || \ | ||
| gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY" | ||
| gh pr edit "$BRANCH_NAME" --add-label "no-release" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same battle-hardening to the VHS job's git operations!
The git and PR operations here need the same robust error handling as suggested for the screengrabs job.
Apply similar error handling improvements as shown in the screengrabs job review above.
.github/workflows/demo.yml
Outdated
| - name: Get Atmos version | ||
| id: get-version | ||
| run: | | ||
| VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) | ||
| echo "version=$VERSION" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: List all .tape files | ||
| id: list-tapes | ||
| run: | | ||
| files=$(find . -type f -name '*.tape') | ||
| echo "matrix=$(echo -n \"$files\" | jq -R -s -c 'split(\"\\n\") | map(select(. != \"\"))')" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Shield your commands from the chaos of spaces and failures!
The shell commands need additional error handling and proper quoting:
Apply these improvements:
- name: Get Atmos version
id: get-version
run: |
- VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
+ VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
+ if [ -z "$VERSION" ]; then
+ echo "Failed to get Atmos version"
+ exit 1
+ fi
echo "version=$VERSION" >> $GITHUB_OUTPUT
- name: List all .tape files
id: list-tapes
run: |
- files=$(find . -type f -name '*.tape')
- echo "matrix=$(echo -n \"$files\" | jq -R -s -c 'split(\"\\n\") | map(select(. != \"\"))')" >> $GITHUB_OUTPUT
+ files=$(find . -type f -name '*.tape' -print0 | jq -R -s -c 'split("\u0000") | map(select(. != ""))')
+ echo "matrix=$files" >> $GITHUB_OUTPUT📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get Atmos version | |
| id: get-version | |
| run: | | |
| VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| - name: List all .tape files | |
| id: list-tapes | |
| run: | | |
| files=$(find . -type f -name '*.tape') | |
| echo "matrix=$(echo -n \"$files\" | jq -R -s -c 'split(\"\\n\") | map(select(. != \"\"))')" >> $GITHUB_OUTPUT | |
| - name: Get Atmos version | |
| id: get-version | |
| run: | | |
| VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) | |
| if [ -z "$VERSION" ]; then | |
| echo "Failed to get Atmos version" | |
| exit 1 | |
| fi | |
| echo "version=$VERSION" >> $GITHUB_OUTPUT | |
| - name: List all .tape files | |
| id: list-tapes | |
| run: | | |
| files=$(find . -type f -name '*.tape' -print0 | jq -R -s -c 'split("\u0000") | map(select(. != ""))') | |
| echo "matrix=$files" >> $GITHUB_OUTPUT |
🧰 Tools
🪛 actionlint (1.7.4)
20-20: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/demo.yml
Outdated
| - name: Push changes to branch | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | ||
| git checkout -b "$BRANCH_NAME" || git checkout "$BRANCH_NAME" | ||
| git add -A | ||
| git commit -m "chore: update screengrabs for $VERSION" || echo "No changes to commit" | ||
| git push origin "$BRANCH_NAME" | ||
|
|
||
| - name: Create or update PR | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | ||
| PR_TITLE="Update screengrabs for $VERSION" | ||
| PR_BODY="This PR updates the screengrabs for Atmos version $VERSION." | ||
| gh pr view "$BRANCH_NAME" > /dev/null 2>&1 || \ | ||
| gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY" | ||
| gh pr edit "$BRANCH_NAME" --add-label "no-release" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen your git operations with proper error handling!
The git and PR operations need more robust error handling.
Apply these improvements:
- name: Push changes to branch
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
VERSION="${{ needs.prepare.outputs.version }}"
BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION"
- git checkout -b "$BRANCH_NAME" || git checkout "$BRANCH_NAME"
+ if ! git checkout -b "$BRANCH_NAME" 2>/dev/null; then
+ if ! git checkout "$BRANCH_NAME"; then
+ echo "Failed to checkout branch"
+ exit 1
+ fi
+ fi
git add -A
- git commit -m "chore: update screengrabs for $VERSION" || echo "No changes to commit"
+ if ! git diff --staged --quiet; then
+ git commit -m "chore: update screengrabs for $VERSION"
+ if ! git push origin "$BRANCH_NAME"; then
+ echo "Failed to push changes"
+ exit 1
+ fi
+ else
+ echo "No changes to commit"
+ fi
- git push origin "$BRANCH_NAME"
- name: Create or update PR
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
VERSION="${{ needs.prepare.outputs.version }}"
BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION"
PR_TITLE="Update screengrabs for $VERSION"
PR_BODY="This PR updates the screengrabs for Atmos version $VERSION."
- gh pr view "$BRANCH_NAME" > /dev/null 2>&1 || \
- gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY"
+ if ! gh pr view "$BRANCH_NAME" > /dev/null 2>&1; then
+ if ! gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY"; then
+ echo "Failed to create PR"
+ exit 1
+ fi
+ fi
- gh pr edit "$BRANCH_NAME" --add-label "no-release"
+ if ! gh pr edit "$BRANCH_NAME" --add-label "no-release"; then
+ echo "Failed to add label to PR"
+ exit 1
+ fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Push changes to branch | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | |
| git checkout -b "$BRANCH_NAME" || git checkout "$BRANCH_NAME" | |
| git add -A | |
| git commit -m "chore: update screengrabs for $VERSION" || echo "No changes to commit" | |
| git push origin "$BRANCH_NAME" | |
| - name: Create or update PR | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | |
| PR_TITLE="Update screengrabs for $VERSION" | |
| PR_BODY="This PR updates the screengrabs for Atmos version $VERSION." | |
| gh pr view "$BRANCH_NAME" > /dev/null 2>&1 || \ | |
| gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY" | |
| gh pr edit "$BRANCH_NAME" --add-label "no-release" | |
| - name: Push changes to branch | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | |
| if ! git checkout -b "$BRANCH_NAME" 2>/dev/null; then | |
| if ! git checkout "$BRANCH_NAME"; then | |
| echo "Failed to checkout branch" | |
| exit 1 | |
| fi | |
| fi | |
| git add -A | |
| if ! git diff --staged --quiet; then | |
| git commit -m "chore: update screengrabs for $VERSION" | |
| if ! git push origin "$BRANCH_NAME"; then | |
| echo "Failed to push changes" | |
| exit 1 | |
| fi | |
| else | |
| echo "No changes to commit" | |
| fi | |
| - name: Create or update PR | |
| env: | |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| BRANCH_NAME="chore/update-build-screengrabs-for-$VERSION" | |
| PR_TITLE="Update screengrabs for $VERSION" | |
| PR_BODY="This PR updates the screengrabs for Atmos version $VERSION." | |
| if ! gh pr view "$BRANCH_NAME" > /dev/null 2>&1; then | |
| if ! gh pr create --base main --head "$BRANCH_NAME" --title "$PR_TITLE" --body "$PR_BODY"; then | |
| echo "Failed to create PR" | |
| exit 1 | |
| fi | |
| fi | |
| if ! gh pr edit "$BRANCH_NAME" --add-label "no-release"; then | |
| echo "Failed to add label to PR" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
demo/screengrabs/build-all.sh (1)
50-69: Add error handling to postprocess functionsThe postprocess functions perform multiple sed operations without checking for errors.
Consider adding error handling:
postprocess_ansi() { local file=$1 + local exit_code=0 # Remove noise and clean up the output - $SED '/- Finding latest version of/d' $file - $SED '/- Installed hashicorp/d' $file + $SED '/- Finding latest version of/d' "$file" || exit_code=$? + $SED '/- Installed hashicorp/d' "$file" || exit_code=$? # ... (similar for other operations) + return $exit_code }.github/workflows/demo.yml (1)
42-46: Improve dependency installation reliabilityThe apt-get commands should be more resilient to transient failures.
Apply these improvements:
- - name: Install dependencies + - name: Install dependencies + timeout-minutes: 5 + retries: 3 run: | - sudo apt-get update - sudo apt-get install -y aha util-linux make jq + sudo apt-get update || (sleep 10 && sudo apt-get update) + DEBIAN_FRONTEND=noninteractive sudo apt-get install -y --no-install-recommends aha util-linux make jq
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/demo.yml(1 hunks).github/workflows/vhs.yaml(0 hunks)demo/screengrabs/build-all.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/vhs.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
demo/screengrabs/build-all.sh
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/demo.yml (5)
1-15: LGTM! Well-structured workflow configuration
The workflow triggers and environment setup are comprehensive and appropriate for the demo regeneration use case.
22-33: Previous review comments about command robustness are still applicable
The commands in the prepare job need additional error handling and proper quoting as highlighted in the previous review.
🧰 Tools
🪛 actionlint (1.7.4)
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
63-89: Previous review comments about git operations robustness are still applicable
The git operations and PR creation steps need more robust error handling as highlighted in the previous review.
93-99: LGTM! Well-implemented concurrency control
The matrix strategy with concurrency group and cancel-in-progress settings is well thought out.
111-138: Previous review comments about git operations robustness are still applicable
The git operations and PR creation steps need more robust error handling as highlighted in the previous review.
| # Determine the correct sed syntax based on the operating system | ||
| if [ "$(uname)" = "Darwin" ]; then | ||
| SED="$SED" # macOS requires '' for in-place editing | ||
| else | ||
| SED="sed -i" # Linux does not require '' | ||
| fi |
There was a problem hiding this comment.
Fix the macOS SED variable initialization
The macOS branch of the conditional is not properly initializing the SED variable. It's currently just reassigning the undefined variable to itself.
Apply this fix:
if [ "$(uname)" = "Darwin" ]; then
- SED="$SED" # macOS requires '' for in-place editing
+ SED="sed -i ''" # macOS requires '' for in-place editing
else
SED="sed -i" # Linux does not require ''
fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Determine the correct sed syntax based on the operating system | |
| if [ "$(uname)" = "Darwin" ]; then | |
| SED="$SED" # macOS requires '' for in-place editing | |
| else | |
| SED="sed -i" # Linux does not require '' | |
| fi | |
| # Determine the correct sed syntax based on the operating system | |
| if [ "$(uname)" = "Darwin" ]; then | |
| SED="sed -i ''" # macOS requires '' for in-place editing | |
| else | |
| SED="sed -i" # Linux does not require '' | |
| fi |
| local extension="${command##*.}" # if any... | ||
| local demo_path=../../examples/$demo | ||
| local output_base_file=artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//') | ||
| local output_html=${output_base_file}.html | ||
| local output_ansi=${output_base_file}.ansi | ||
| local output_dir=$(dirname $output_base_file) | ||
| local output_html=${output_base_file}.html | ||
| local output_ansi=${output_base_file}.ansi | ||
| local output_dir=$(dirname $output_base_file) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve variable declarations and path handling
The current implementation has two issues:
- Variables are declared and assigned in one line, which can mask return values
- Path handling could be more robust
Apply these improvements:
- local extension="${command##*.}" # if any...
- local demo_path=../../examples/$demo
- local output_base_file=artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//')
- local output_html=${output_base_file}.html
- local output_ansi=${output_base_file}.ansi
- local output_dir=$(dirname $output_base_file)
+ local extension
+ local demo_path
+ local output_base_file
+ local output_html
+ local output_ansi
+ local output_dir
+
+ extension="${command##*.}"
+ demo_path="../../examples/${demo}"
+ output_base_file="artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//')"
+ output_html="${output_base_file}.html"
+ output_ansi="${output_base_file}.ansi"
+ output_dir="$(dirname "$output_base_file")"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local extension="${command##*.}" # if any... | |
| local demo_path=../../examples/$demo | |
| local output_base_file=artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//') | |
| local output_html=${output_base_file}.html | |
| local output_ansi=${output_base_file}.ansi | |
| local output_dir=$(dirname $output_base_file) | |
| local output_html=${output_base_file}.html | |
| local output_ansi=${output_base_file}.ansi | |
| local output_dir=$(dirname $output_base_file) | |
| local extension | |
| local demo_path | |
| local output_base_file | |
| local output_html | |
| local output_ansi | |
| local output_dir | |
| extension="${command##*.}" | |
| demo_path="../../examples/${demo}" | |
| output_base_file="artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//')" | |
| output_html="${output_base_file}.html" | |
| output_ansi="${output_base_file}.ansi" | |
| output_dir="$(dirname "$output_base_file")" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/demo.yml (1)
3-9: LGTM! Consider optimizing PR triggers.The workflow triggers are well-configured for both manual and automated runs. The path filter helps prevent unnecessary executions.
Consider adding a label condition to prevent running on every PR:
pull_request: types: [labeled, opened, synchronize] + if: contains(github.event.pull_request.labels.*.name, 'update-demos') paths: - ".github/workflows/demo.yml"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)demo/screengrabs/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/screengrabs/Makefile
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
76-76: the runner of "peter-evans/create-pull-request@v5" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
109-109: shellcheck reported issue in this script: SC2129:style:3:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
127-127: the runner of "peter-evans/create-pull-request@v5" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/demo.yml (1)
22-32: 🛠️ Refactor suggestion
Strengthen shell commands with proper error handling and quoting!
The commands need to be more resilient to failures and properly quoted.
Apply these improvements:
- name: Get Atmos version
id: get-version
run: |
- VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
+ VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
+ if [[ -z "$VERSION" ]]; then
+ echo "Failed to fetch Atmos version"
+ exit 1
+ fi
echo "version=$VERSION" >> $GITHUB_OUTPUT
- name: List all .tape files
id: list-tapes
run: |
- files=$(find . -type f -name '*.tape')
- echo "matrix=$(echo -n \"$files\" | jq -R -s -c 'split(\"\\n\") | map(select(. != \"\"))')" >> $GITHUB_OUTPUT
+ if ! files=$(find . -type f -name '*.tape' -print0); then
+ echo "Failed to find .tape files"
+ exit 1
+ fi
+ matrix=$(jq -R -s -c 'split("\u0000") | map(select(. != ""))' <<< "$files")
+ echo "matrix=$matrix" >> $GITHUB_OUTPUTLikely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/demo.yml
Outdated
| - name: Prepare variables | ||
| id: vars | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| JOB_NAME=$(basename "${{ matrix.file }}" | sed 's/.tape$//') | ||
| echo "version=${VERSION}" >> $GITHUB_OUTPUT | ||
| echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT | ||
| echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve variable preparation robustness!
The variable preparation step needs better error handling and validation.
- name: Prepare variables
id: vars
run: |
VERSION="${{ needs.prepare.outputs.version }}"
- JOB_NAME=$(basename "${{ matrix.file }}" | sed 's/.tape$//')
+ if [[ ! -f "${{ matrix.file }}" ]]; then
+ echo "Tape file not found: ${{ matrix.file }}"
+ exit 1
+ fi
+ JOB_NAME=$(basename "${{ matrix.file }}" .tape)
+ if [[ -z "$JOB_NAME" ]]; then
+ echo "Failed to extract job name"
+ exit 1
+ fi
- echo "version=${VERSION}" >> $GITHUB_OUTPUT
- echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT
- echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT
+ {
+ echo "version=${VERSION}"
+ echo "job_name=${JOB_NAME}"
+ echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}"
+ } >> "$GITHUB_OUTPUT"📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Prepare variables | |
| id: vars | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| JOB_NAME=$(basename "${{ matrix.file }}" | sed 's/.tape$//') | |
| echo "version=${VERSION}" >> $GITHUB_OUTPUT | |
| echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT | |
| echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT | |
| - name: Prepare variables | |
| id: vars | |
| run: | | |
| VERSION="${{ needs.prepare.outputs.version }}" | |
| if [[ ! -f "${{ matrix.file }}" ]]; then | |
| echo "Tape file not found: ${{ matrix.file }}" | |
| exit 1 | |
| fi | |
| JOB_NAME=$(basename "${{ matrix.file }}" .tape) | |
| if [[ -z "$JOB_NAME" ]]; then | |
| echo "Failed to extract job name" | |
| exit 1 | |
| fi | |
| { | |
| echo "version=${VERSION}" | |
| echo "job_name=${JOB_NAME}" | |
| echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" | |
| } >> "$GITHUB_OUTPUT" |
🧰 Tools
🪛 actionlint (1.7.4)
109-109: shellcheck reported issue in this script: SC2129:style:3:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
109-109: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
osterman
force-pushed
the
regen-screengrabs
branch
from
9575eae to
d27fceb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
demo/recordings/style.tape (1)
15-17: Consider merging Margin and Padding for consistency.You set the margin to zero and then redefine margin again at line 28. It might be simpler to keep margin settings in one place unless you intend to override them later.
demo/screengrabs/build-all.sh (1)
15-20: Declare and assign separately to avoid masking return values.ShellCheck SC2155 advises caution.
Here's a minimal example fix:
- local extension="${command##*.}" # if any... - local demo_path=../../examples/$demo - local output_base_file=artifacts/$(echo "$command" | sed -E 's/ -/-/g' | sed -E 's/ +/-/g' | sed 's/---/--/g' | sed 's/scripts\///' | sed 's/\.sh$//') - local output_html=${output_base_file}.html - local output_ansi=${output_base_file}.ansi - local output_dir=$(dirname $output_base_file) + local extension + extension="${command##*.}" + local demo_path + demo_path="../../examples/$demo" + ...🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
.github/workflows/demo.yml (2)
24-26: Double-quote your variable expansions to avoid word splitting.When injecting output into the environment, wrap $VERSION in quotes to prevent unexpected splitting.
Here's an illustrative fix:
-VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) +VERSION="$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)"🧰 Tools
🪛 actionlint (1.7.4)
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
31-32: Apply quotes around $files to avoid potential word splitting.Enclose expansions in quotes for robust handling of spaces or special characters.
-files=$(find . -type f -name '*.tape') +files="$(find . -type f -name '*.tape')"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/demo.yml(1 hunks).github/workflows/vhs.yaml(0 hunks)demo/defaults.tape(1 hunks)demo/recordings/demo.tape(1 hunks)demo/recordings/style.tape(1 hunks)demo/screengrabs/Makefile(1 hunks)demo/screengrabs/build-all.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/vhs.yaml
✅ Files skipped from review due to trivial changes (1)
- demo/defaults.tape
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/screengrabs/Makefile
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
24-24: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:26: Double quote to prevent globbing and word splitting
(shellcheck)
30-30: shellcheck reported issue in this script: SC2086:info:2:96: Double quote to prevent globbing and word splitting
(shellcheck)
103-103: shellcheck reported issue in this script: SC2129:style:3:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Shellcheck (0.10.0)
demo/screengrabs/build-all.sh
[warning] 17-17: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (9)
demo/recordings/style.tape (2)
3-14: Good use of modular settings for VHS aesthetics.
These lines establish consistent theme, font, and window styling. It's neat to see all defaults centralized, improving maintainability.
22-36: All defaults appear consistent across the file.
Retaining uniform definitions for frame rate, cursor blink, and margin fill ensures cohesive styling without confusion.
demo/recordings/demo.tape (4)
1-2: Intro lines clearly frame the purpose of the demo.
Your approach for referencing the style file exemplifies a DRY pattern: changes in styling remain centralized.
6-10: Sleep intervals are well-structured for demonstration clarity.
The timing helps highlight each command's output.
59-64: Scrolling behavior clarifies large outputs.
Scrolling through 25 lines at a time is practical for capturing partial screen content. This is great for longer outputs.
67-85: All-around thorough demonstration of key Atmos features.
Commands show a broad range of capabilities, from listing stacks to viewing workflows. Very comprehensive.
demo/screengrabs/build-all.sh (2)
50-64: Noise filtering is nicely handled.
The commands remove lines irrelevant to demo clarity. This reduces visual clutter.
5-10:
Mac-specific SED usage is incomplete.
Currently, SED="$SED" is not setting the variable. If you wish to support in-place editing on macOS, you must set it properly.
Here's a proposed fix:
if [ "$(uname)" = "Darwin" ]; then
- SED="$SED" # macOS requires '' for in-place editing
+ SED="sed -i ''" # macOS requires '' for in-place editing
else
SED="sed -i" # Linux does not require ''
fiLikely invalid or redundant comment.
.github/workflows/demo.yml (1)
103-109: 🛠️ Refactor suggestion
Consider grouping echo statements to streamline output redirection.
ShellCheck suggests grouping multiple echo commands inside braces.
-echo "version=${VERSION}" >> $GITHUB_OUTPUT
-echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT
-echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT
+{
+ echo "version=${VERSION}"
+ echo "job_name=${JOB_NAME}"
+ echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}"
+} >> "$GITHUB_OUTPUT"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
103-103: shellcheck reported issue in this script: SC2129:style:3:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:4:32: Double quote to prevent globbing and word splitting
(shellcheck)
103-103: shellcheck reported issue in this script: SC2086:info:5:63: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
demo/recordings/studio.go (2)
31-32: Remove unused constant.The
vhsTimeoutconstant is defined but never used in the code. Consider either implementing the timeout functionality or removing the constant.-// Timeout for VHS processing -const vhsTimeout = 10 * time.Minute🧰 Tools
🪛 golangci-lint (1.62.2)
32-32: const
vhsTimeoutis unused(unused)
330-346: Improve error handling in duration calculation.Instead of returning -1 for errors, consider returning an error along with the duration value, which is more idiomatic in Go.
-func GetMP4Duration(mp4File string) float64 { +func GetMP4Duration(mp4File string) (float64, error) { cmd := exec.Command("ffprobe", "-v", "error", "-show_entries", "format=duration", "-of", "csv=p=0", mp4File) output, err := cmd.Output() if err != nil { log.Debug("Failed to get video duration", "file", ConvertToRelativeFromCWD(mp4File), "error", err) - return -1 + return 0, fmt.Errorf("failed to get video duration: %w", err) } duration, err := strconv.ParseFloat(strings.TrimSpace(string(output)), 64) if err != nil { log.Debug("Failed to parse video duration", "file", ConvertToRelativeFromCWD(mp4File), "error", err) - return -1 + return 0, fmt.Errorf("failed to parse video duration: %w", err) } - return duration + return duration, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)demo/recordings/studio.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/demo.yml
🧰 Additional context used
🪛 golangci-lint (1.62.2)
demo/recordings/studio.go
32-32: const vhsTimeout is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: vhs
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
demo/recordings/studio.go (1)
304-307: Prevent short-circuiting the entire scene loop.Using
return nilhere stops processing remaining scenes if one is up-to-date. Switch tocontinueto skip only the current scene without exiting the loop.if isUpToDate(outputGif, outputMp4WithAudio) { log.Info("Skipping GIF generation", "file", ConvertToRelativeFromCWD(outputGif), "reason", "already up-to-date") - return nil + continue } else {
| _, err := RunCmdWithSpinner("Cleaning up generated files...", exec.Command("rm", "-rf", mp4OutDir, gifOutDir)) | ||
| return err |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation prompt for destructive operations.
The clean command uses rm -rf without confirmation, which could be dangerous. Consider adding a confirmation prompt before deleting files.
RunE: func(cmd *cobra.Command, args []string) error {
+ fmt.Printf("This will remove all generated files in:\n%s\n%s\nAre you sure? [y/N] ", mp4OutDir, gifOutDir)
+ var response string
+ fmt.Scanln(&response)
+ if strings.ToLower(response) != "y" {
+ return fmt.Errorf("operation cancelled by user")
+ }
_, err := RunCmdWithSpinner("Cleaning up generated files...", exec.Command("rm", "-rf", mp4OutDir, gifOutDir))
return err
},📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := RunCmdWithSpinner("Cleaning up generated files...", exec.Command("rm", "-rf", mp4OutDir, gifOutDir)) | |
| return err | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| fmt.Printf("This will remove all generated files in:\n%s\n%s\nAre you sure? [y/N] ", mp4OutDir, gifOutDir) | |
| var response string | |
| fmt.Scanln(&response) | |
| if strings.ToLower(response) != "y" { | |
| return fmt.Errorf("operation cancelled by user") | |
| } | |
| _, err := RunCmdWithSpinner("Cleaning up generated files...", exec.Command("rm", "-rf", mp4OutDir, gifOutDir)) | |
| return err | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
demo/recordings/studio.go (2)
71-74:⚠️ Potential issueAdd confirmation prompt for destructive operations.
The
cleancommand usesrm -rfwithout confirmation, which could accidentally delete important files.RunE: func(cmd *cobra.Command, args []string) error { + fmt.Printf("This will remove all generated files in:\n%s\n%s\nAre you sure? [y/N] ", mp4OutDir, gifOutDir) + var response string + fmt.Scanln(&response) + if strings.ToLower(response) != "y" { + return fmt.Errorf("operation cancelled by user") + } _, err := RunCmdWithSpinner("Cleaning up generated files...", exec.Command("rm", "-rf", mp4OutDir, gifOutDir)) return err },
304-307:⚠️ Potential issueFix incorrect return statement in loop.
Using
return nilhere stops processing remaining scenes if one is up-to-date. This is likely a bug.if isUpToDate(outputGif, outputMp4WithAudio) { log.Info("Skipping GIF generation", "file", ConvertToRelativeFromCWD(outputGif), "reason", "already up-to-date") - return nil + continue }
🧹 Nitpick comments (3)
demo/recordings/studio.go (3)
32-32: Remove unused constant.The
vhsTimeoutconstant is defined but never used in the code.-// Timeout for VHS processing -const vhsTimeout = 10 * time.Minute🧰 Tools
🪛 golangci-lint (1.62.2)
32-32: const
vhsTimeoutis unused(unused)
433-442: Add cleanup for temporary palette file.The palette file generated for GIF creation is not cleaned up after use, which could lead to disk space issues over time.
func createGif(inputMp4, outputGif string) error { palette := outputGif + "-palette.png" + defer os.Remove(palette) // Clean up temporary palette file // Generate palette for better GIF quality, only if outdated if !isUpToDate(palette, inputMp4) {
445-447: Consider making FPS configurable.The GIF frame rate is hardcoded to 10 FPS. Consider making this configurable to allow for different quality/size trade-offs.
+// DefaultGifFPS is the default frames per second for GIF output +const DefaultGifFPS = 10 + func createGif(inputMp4, outputGif string) error { palette := outputGif + "-palette.png" // Create GIF using the palette exitCode, err := RunCmdWithSpinner("Creating GIF...", exec.Command( - "ffmpeg", "-i", inputMp4, "-i", palette, "-lavfi", "fps=10 [video]; [video][1:v] paletteuse", "-y", outputGif)) + "ffmpeg", "-i", inputMp4, "-i", palette, "-lavfi", fmt.Sprintf("fps=%d [video]; [video][1:v] paletteuse", DefaultGifFPS), "-y", outputGif))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo/recordings/studio.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
demo/recordings/studio.go
32-32: const vhsTimeout is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: vhs
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
| if exitCode, err = RunCmdWithSpinner(fmt.Sprintf("Recording %s to mp4...", baseName), exec.Command("vhs", tape, "--output", outputMp4)); err != nil || exitCode != 0 { | ||
| log.Error("Failed to record tape", "tape", ConvertToRelativeFromCWD(tape), "file", ConvertToRelativeFromCWD(outputMp4), "error", err) | ||
| os.Exit(exitCode) | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider graceful error handling instead of immediate exit.
Using os.Exit in a library function is generally not recommended as it prevents proper cleanup and error handling by the caller.
if exitCode, err = RunCmdWithSpinner(fmt.Sprintf("Recording %s to mp4...", baseName), exec.Command("vhs", tape, "--output", outputMp4)); err != nil || exitCode != 0 {
log.Error("Failed to record tape", "tape", ConvertToRelativeFromCWD(tape), "file", ConvertToRelativeFromCWD(outputMp4), "error", err)
- os.Exit(exitCode)
+ return fmt.Errorf("failed to record tape %s: %w", tape, err)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if exitCode, err = RunCmdWithSpinner(fmt.Sprintf("Recording %s to mp4...", baseName), exec.Command("vhs", tape, "--output", outputMp4)); err != nil || exitCode != 0 { | |
| log.Error("Failed to record tape", "tape", ConvertToRelativeFromCWD(tape), "file", ConvertToRelativeFromCWD(outputMp4), "error", err) | |
| os.Exit(exitCode) | |
| } else { | |
| if exitCode, err = RunCmdWithSpinner(fmt.Sprintf("Recording %s to mp4...", baseName), exec.Command("vhs", tape, "--output", outputMp4)); err != nil || exitCode != 0 { | |
| log.Error("Failed to record tape", "tape", ConvertToRelativeFromCWD(tape), "file", ConvertToRelativeFromCWD(outputMp4), "error", err) | |
| return fmt.Errorf("failed to record tape %s: %w", tape, err) | |
| } else { |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/demo.yml (3)
158-162: 🛠️ Refactor suggestionSet Swap Space Action Version Pinning
The swap space step currently uses a commit SHA forpierotofy/set-swap-space. For better maintainability, it’s recommended to pin to an official version tag (e.g.,v1.0).- uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c + uses: pierotofy/[email protected]
183-190:⚠️ Potential issueUndefined JOB_NAME Variable in Prepare Variables Step
ThePrepare variablesstep referencesJOB_NAMEwithout initializing it, which can lead to runtime errors. DefineJOB_NAME(for example, set it to"demo") before using it in the output export.- echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT - echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT + JOB_NAME="demo" + echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT + echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT
211-218: 🛠️ Refactor suggestionUploading Assets to S3
The S3 synchronization block currently executesaws s3commands without explicit error checks. To ensure that demo artifacts are reliably uploaded, wrap eachaws s3 syncandaws s3 lscommand in anifblock to verify success. For example:-for ext in mp4 gif; do - aws s3 sync $ext/ s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/ - aws s3 ls s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/ --recursive --human-readable --summarize -done +for ext in mp4 gif; do + if ! aws s3 sync "$ext/" "s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/"; then + echo "Failed to sync ${ext} files to S3" + exit 1 + fi + if ! aws s3 ls "s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/" --recursive --human-readable --summarize; then + echo "Failed to verify ${ext} files in S3" + exit 1 + fi +done
🧹 Nitpick comments (6)
.github/workflows/demo.yml (6)
1-9: Workflow Triggers and Dispatch
The workflow is set to trigger manually viaworkflow_dispatchand on pull request events for the workflow file. Given the demo regeneration objective, consider expanding the trigger paths (for example, includingdemo/**,docs/demo.gif, andwebsite/static/img/demo.gif) so that changes in demo assets also initiate this workflow.
23-32: Prepare Job – Atmos Version Retrieval
Thepreparejob fetches the Atmos version using acurlandjqpipeline, and then exports it through the GitHub Actions output. Although the built-in error handling is usually sufficient, you may want to add a quick check to ensure that the version is not empty in case of an API hiccup.
177-182: Atmos Testing Validation
The test step for Atmos checks the existence of the executable and displays version/help information. For additional assurance, you might consider validating the command output to ensure it reflects the expected version.
203-210: Screencast Recording Step
The recording step executes multiple commands (checking ffmpeg, running the build with Go, copying the resulting gif, and staging it via git). While GitHub Actions will fail on any non-zero command, consider adding conditional checks to explicitly catch failures in thego run studio.go buildstep, if desired.
235-243: Adding Image to Job Summary
Appending demo images to the job summary works correctly. For enhanced robustness, consider wrapping the echo commands in braces and quoting$GITHUB_STEP_SUMMARYto prevent any potential issues with word splitting.
244-247: No Changes Message
The final step in the workflow that echoes "No changes to demo" is clear. A minor improvement would be to use proper quoting for$GITHUB_STEP_SUMMARYfor consistency with other steps.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/demo.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/demo.yml (2)
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:42-45
Timestamp: 2025-01-11T02:36:29.367Z
Learning: In GitHub Actions workflows, avoid adding redundant error handling as the platform already handles non-zero exit codes and provides helpful error messages by default.
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:119-120
Timestamp: 2025-01-10T23:23:38.968Z
Learning: AWS CLI commands like `aws sts get-caller-identity` have built-in error handling and will fail with a non-zero exit code, making explicit error handling redundant in CI/CD workflows.
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: screengrabs
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (15)
.github/workflows/demo.yml (15)
10-14: Permissions Configuration
The permissions block is correctly defined with appropriate write access to contents, id-token, and pull-requests.
15-21: Global Environment Variables
The environment variables are clearly set. In the future, you might consider parameterizing constants such asTERRAFORM_VERSIONfor flexibility, but this setup is solid for now.
37-89: Screengrabs Job Structure
Thescreengrabsjob is well structured: dependency installation, repository checkout, Atmos installation, Terraform setup, building via make, and PR creation. The steps flow logically and use up-to-date actions likepeter-evans/create-pull-request@v7.
97-100: VHS Job Environment Settings
The environmental settings (including timeout, environment, and AWS_REGION) for thevhsjob look appropriate for the intended context.
101-125: Dynamic Environment Variable Setup
The step that sets environment variables based on the event type (release vs. pull_request) is implemented clearly. Make sure that each value (such as bucket names and base URLs) aligns with the intended deployment targets.
126-133: AWS Credentials Configuration
Usingaws-actions/configure-aws-credentials@v4correctly to configure AWS credentials ensures secure interactions with AWS services.
134-137: AWS Identity Test
The test usingaws sts get-caller-identityis sufficient to verify that the AWS credentials and role assumption are working as expected.
138-145: GitHub App Token Retrieval
The step that creates a GitHub App token is clear and follows best practices.
146-152: Repository Checkout for Assets
Usingactions/checkout@v4to check out thecloudposse-corp/assetsrepository is implemented correctly.
154-157: Audio Track Installation
Copying the background audio track for the demo is straightforward and clear.
163-171: Atmos Installation for VHS Job
The installation of Atmos in the VHS job mirrors the earlier setup and is consistent.
172-176: Terraform Setup for VHS Job
The Terraform setup step usinghashicorp/setup-terraform@v3is configured correctly with the designated version.
191-196: VHS Setup Action
The VHS setup step usingcharmbracelet/vhs-action@v2is correctly implemented and requires no changes.
197-202: FFmpeg Setup
The FFmpeg installation step viaFedericoCarboni/setup-ffmpeg@v3is configured to retrieve the latest release. This ensures you have the most up-to-date functionality for transcoding operations.
220-234: PR Creation for VHS Assets
The step that creates or updates the PR usingpeter-evans/create-pull-request@v7is well configured. Just ensure that the branch naming (which uses the variables set earlier) reflects the intended changes; this ties back to theJOB_NAMEfix.
.github/workflows/demo.yml
Outdated
| vhs: | ||
| needs: [prepare] | ||
| #runs-on: ubuntu-24.04 | ||
| runs-on: | ||
| - runs-on=${{ github.run_id }} | ||
| - runner=large | ||
| # Longer timeouts typically indicate VHS has hung |
There was a problem hiding this comment.
Runner Specification in VHS Job
The vhs job’s runs-on section is unconventional. The entry runs-on=${{ github.run_id }} along with runner=large does not match the standard runner labels (e.g., ubuntu-latest, ubuntu-24.04, or self-hosted). Please verify if these are meant to be custom labels; otherwise, consider switching to a supported label.
🧰 Tools
🪛 actionlint (1.7.4)
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/demo.yml (3)
184-191:⚠️ Potential issuePrepare Variables – Undefined JOB_NAME.
This step echoes a value for
job_nameusing the variable${JOB_NAME}, yetJOB_NAMEis not defined anywhere. This will likely cause the workflow to fail. DefineJOB_NAMEexplicitly (for example, setJOB_NAME="demo") before using it:+ JOB_NAME="demo"
245-248: 🛠️ Refactor suggestionNo Changes Step – Output Consistency.
Ensure that echoing "No changes to demo" also properly quotes
GITHUB_STEP_SUMMARY:- echo "No changes to demo" >> $GITHUB_STEP_SUMMARY + echo "No changes to demo" >> "$GITHUB_STEP_SUMMARY"
236-244: 🛠️ Refactor suggestionJob Summary Update – Adding Demo Assets.
Appending the demo GIF and video link to
GITHUB_STEP_SUMMARYshould use proper quoting to prevent issues with word splitting. For example:- echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo GIF" + echo "" + } >> "$GITHUB_STEP_SUMMARY"Apply a similar approach for the subsequent echo commands.
🧹 Nitpick comments (5)
.github/workflows/demo.yml (5)
1-9: Workflow Name & Trigger Configuration.The workflow name "Demo" is clear, and the manual
workflow_dispatchalong with the pull request trigger for changes to.github/workflows/demo.ymlis correctly set up. Consider expanding the file trigger paths to include demo-related files (e.g.,"demo/**","docs/demo.gif","website/static/img/demo.gif") if you intend for this workflow to run when those change.
90-99: VHS Job – Runner Configuration.The VHS job is configured to run on
ubuntu-24.04. Note the commented-out alternative runner configuration (lines 93–95) which uses non‑standard labels likerunner=large. If these alternatives are no longer necessary, consider removing them to keep the workflow clean.🧰 Tools
🪛 actionlint (1.7.4)
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
159-163: Set Swap Space Action.Consider pinning the
set-swap-spaceaction to a stable version tag rather than a commit SHA for easier maintenance. For example:- uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c + uses: pierotofy/[email protected]
178-183: Test Atmos.The test step running
which atmos,atmos version, andatmos --helpconfirms that Atmos is installed. Optionally, you could add output validation to ensure Atmos behaves as expected.
212-219: Uploading Assets to S3.The loop over file extensions (
mp4andgif) syncs assets to S3 and lists the contents. For enhanced robustness, consider quoting variable references to avoid issues with shell word splitting. For example:- aws s3 sync $ext/ s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/ + aws s3 sync "$ext/" "s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/demo.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/demo.yml (2)
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:42-45
Timestamp: 2025-01-11T02:36:29.367Z
Learning: In GitHub Actions workflows, avoid adding redundant error handling as the platform already handles non-zero exit codes and provides helpful error messages by default.
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:119-120
Timestamp: 2025-01-10T23:23:38.968Z
Learning: AWS CLI commands like `aws sts get-caller-identity` have built-in error handling and will fail with a non-zero exit code, making explicit error handling redundant in CI/CD workflows.
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: vhs
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (17)
.github/workflows/demo.yml (17)
10-14: Permissions Configuration.The permissions are appropriately set to allow
writeaccess for contents, id-token, and pull-requests.
15-21: Environment Variables Setup.The environment variables such as
TERM,COLORTERM,LANG,ATMOS_LOGS_LEVEL, andTERRAFORM_VERSIONare well defined.
23-36: Prepare Job – Atmos Version Retrieval.The prepare job checks out the repository and retrieves the Atmos version via a curl command. While the current implementation is concise and relies on GitHub Actions’ built‑in failure handling, consider verifying that the version output is valid if any issues arise.
37-73: Screengrabs Job – Dependencies, Build, & Checkout.The screengrabs job installs necessary dependencies, sets Git preferences, checks out the repository, installs Atmos, and sets up Terraform. The build step (
make -C demo/screengrabs build-all install) is straightforward. Since GitHub Actions naturally aborts on non‑zero exit codes, additional error handling isn’t strictly required.
75-88: Screengrabs Job – PR Creation.The PR creation step using
peter-evans/create-pull-request@v7is well implemented with clear branch naming and commit message conventions.
100-108: VHS Job – Environment Setup & Tmate Session.Setting
AWS_REGIONin the job’s environment and configuring the tmate session viamxschmitt/action-tmate@v3is correctly implemented.
109-125: VHS Job – Dynamic Environment Variables.The conditional logic for setting environment variables based on the event (release vs. pull_request) is clear and effectively assigns IAM roles, S3 bucket details, and the BASE_URL.
127-134: AWS Credentials Configuration.Configuring AWS credentials using
aws-actions/configure-aws-credentials@v4with the necessary parameters is correctly implemented.
135-138: AWS Verification.The step running
aws sts get-caller-identitysufficiently validates AWS credentials.
139-153: GitHub App Token & Repository Checkout.The steps to create a GitHub app token and check out the
cloudposse-corp/assetsrepository are implemented correctly and set up the context for subsequent operations.
155-158: Installing the Audio Track.Copying the background audio file from the assets repository into
demo/recordingsis simple and effective.
164-172: Install Atmos in VHS Job.This step mirrors the Atmos installation in the screengrabs job and is implemented consistently.
173-177: Terraform Setup in VHS Job.The terraform setup step using
hashicorp/setup-terraform@v3is clear and leverages the environment variable for the Terraform version.
192-197: Setup VHS Action.The VHS setup using
charmbracelet/vhs-action@v2is correctly configured.
198-203: FFmpeg Setup.The step setting up FFmpeg via
FedericoCarboni/setup-ffmpeg@v3is implemented correctly with parameters ensuring the latest release is used.
204-211: Record Screencast.This step verifies the ffmpeg installation, builds the screencast with
go run studio.go build, and moves the generated GIF to the docs folder. It’s concise; you might consider adding a check to ensure thatdocs/demo.gifexists before proceeding with the Git add.
221-235: VHS Job – PR Creation.The pull request creation step is consistent with the screengrabs job and is implemented cleanly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/tui/viewport/tui.go (3)
22-39: Enhance constant documentation.The comment for
maxLogLinescould be more descriptive about its purpose and impact on memory usage.-const maxLogLines = 25 // 🔥 Track max number of lines +const maxLogLines = 25 // Maximum number of log lines to keep in memory to prevent unbounded growth
74-77: Improve error handling for command start.The error message could be more descriptive for debugging purposes.
if err := cmd.Start(); err != nil { - log.Debug("failed to start command", "error", err) + log.Debug("failed to start command", + "error", err, + "command", cmd.String(), + "args", cmd.Args, + ) return 1, err }
293-305: Optimize text wrapping for long lines.Consider using strings.Builder for better performance with long text.
func wrapText(text string, width int) []string { if width <= 0 { return []string{text} // Prevent division by zero } var lines []string + var builder strings.Builder + builder.Grow(len(text)) for len(text) > width { lines = append(lines, text[:width]) // Store a chunk text = text[width:] // Remove chunk from original text } lines = append(lines, text) // Add the last piece return lines }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)internal/tui/viewport/tui.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/demo.yml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: vhs
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/tui/viewport/tui.go (3)
1-20: Well-structured imports and package organization!The imports are logically grouped and include all necessary dependencies for terminal UI implementation.
127-142: Fix concurrency data race on shared slice.The concurrent access to logLines from multiple goroutines is still unsafe.
273-291: Add error handling for viewport content updates.The viewport width validation is still missing, which could lead to rendering issues.
| func RunWithSpinner(title string, fn func(chan string, *[]string) (int, error)) (*Model, error) { | ||
| m := newModel(title, fn) | ||
|
|
||
| // Conditionally disable input handling when there's no TTY | ||
| opts := []tea.ProgramOption{} | ||
| if !m.hasTTY { | ||
| log.Debug("degrading to headless input/output handling") | ||
| opts = append(opts, | ||
| tea.WithInput(strings.NewReader("")), // 🚀 Prevents input handling | ||
| tea.WithOutput(os.Stderr), // 🚀 Prevents UI rendering to TTY | ||
| ) | ||
| } | ||
| p := tea.NewProgram(m, opts...) | ||
|
|
||
| updatedModel, err := p.Run() // ✅ Get the final updated model | ||
| finalModel := updatedModel.(Model) // ✅ Type assertion to model | ||
|
|
||
| return &finalModel, err // ✅ Return the correct model instance | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for model type assertion.
The type assertion on line 56 could panic if the model is not of the expected type.
- finalModel := updatedModel.(Model) // ✅ Type assertion to model
+ finalModel, ok := updatedModel.(Model)
+ if !ok {
+ return nil, fmt.Errorf("unexpected model type: %T", updatedModel)
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RunWithSpinner(title string, fn func(chan string, *[]string) (int, error)) (*Model, error) { | |
| m := newModel(title, fn) | |
| // Conditionally disable input handling when there's no TTY | |
| opts := []tea.ProgramOption{} | |
| if !m.hasTTY { | |
| log.Debug("degrading to headless input/output handling") | |
| opts = append(opts, | |
| tea.WithInput(strings.NewReader("")), // 🚀 Prevents input handling | |
| tea.WithOutput(os.Stderr), // 🚀 Prevents UI rendering to TTY | |
| ) | |
| } | |
| p := tea.NewProgram(m, opts...) | |
| updatedModel, err := p.Run() // ✅ Get the final updated model | |
| finalModel := updatedModel.(Model) // ✅ Type assertion to model | |
| return &finalModel, err // ✅ Return the correct model instance | |
| } | |
| func RunWithSpinner(title string, fn func(chan string, *[]string) (int, error)) (*Model, error) { | |
| m := newModel(title, fn) | |
| // Conditionally disable input handling when there's no TTY | |
| opts := []tea.ProgramOption{} | |
| if !m.hasTTY { | |
| log.Debug("degrading to headless input/output handling") | |
| opts = append(opts, | |
| tea.WithInput(strings.NewReader("")), // 🚀 Prevents input handling | |
| tea.WithOutput(os.Stderr), // 🚀 Prevents UI rendering to TTY | |
| ) | |
| } | |
| p := tea.NewProgram(m, opts...) | |
| updatedModel, err := p.Run() // ✅ Get the final updated model | |
| finalModel, ok := updatedModel.(Model) | |
| if !ok { | |
| return nil, fmt.Errorf("unexpected model type: %T", updatedModel) | |
| } | |
| return &finalModel, err // ✅ Return the correct model instance | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
.github/workflows/demo.yml (6)
3-9: Expand Workflow Trigger Paths
The workflow currently triggers only on changes to.github/workflows/demo.yml. Consider adding additional paths (e.g.,demo/**,docs/demo.gif,website/static/img/demo.gif) so that changes to demo assets also trigger this workflow.
28-32: Improve Atmos Version Retrieval
To make the Atmos version fetching more robust, consider using the--failflag withcurland quoting the$GITHUB_OUTPUTvariable. For example:- VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) - echo "version=$VERSION" >> $GITHUB_OUTPUT + VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) + echo "version=$VERSION" >> "$GITHUB_OUTPUT"
159-163: Pin the Set Swap Space Action Version
Currently, the swap space action is referenced using a commit SHA. For better maintainability and clarity, update this to use an official version tag (e.g.,v1.0).
213-221: Quote Variables in S3 Sync Commands
In the S3 upload loop, variable expansions (e.g.,$ext,${{ env.S3_BUCKET_NAME }}, and${{ env.S3_PATH }}) aren’t quoted, which may lead to word splitting or globbing issues. Consider updating these commands as follows:- aws s3 sync $ext/ s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/ - aws s3 ls s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/ --recursive --human-readable --summarize + aws s3 sync "$ext/" "s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/" + aws s3 ls "s3://${{ env.S3_BUCKET_NAME }}/${{ env.S3_PATH }}/${ext}/" --recursive --human-readable --summarize
237-242: Quote the GITHUB_STEP_SUMMARY Variable
To avoid issues with word splitting, please enclose$GITHUB_STEP_SUMMARYin quotes when appending withecho. For example:- echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo GIF" + echo "" + } >> "$GITHUB_STEP_SUMMARY"
246-250: Consistently Quote GITHUB_STEP_SUMMARY in All Steps
Similar to the previous note, please also enclose$GITHUB_STEP_SUMMARYin quotes in the "No changes" step to ensure consistent behavior.- echo "No changes to demo" >> $GITHUB_STEP_SUMMARY + echo "No changes to demo" >> "$GITHUB_STEP_SUMMARY"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧠 Learnings (1)
.github/workflows/demo.yml (2)
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:42-45
Timestamp: 2025-01-11T02:36:29.367Z
Learning: In GitHub Actions workflows, avoid adding redundant error handling as the platform already handles non-zero exit codes and provides helpful error messages by default.
Learnt from: osterman
PR: cloudposse/atmos#872
File: .github/workflows/demo.yml:119-120
Timestamp: 2025-01-10T23:23:38.968Z
Learning: AWS CLI commands like `aws sts get-caller-identity` have built-in error handling and will fail with a non-zero exit code, making explicit error handling redundant in CI/CD workflows.
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: vhs
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
.github/workflows/demo.yml
Outdated
| runs-on: | ||
| - runs-on=${{ github.run_id }} | ||
| - runner=large |
There was a problem hiding this comment.
Review Runner Specification in VHS Job
The runs-on field in the vhs job uses non-standard labels:
runs-on=${{ github.run_id }}runner=large
These labels are unconventional. Please verify that these are intended for your custom self-hosted runner setup or consider switching to standard labels (e.g.,ubuntu-24.04orubuntu-latest).
🧰 Tools
🪛 actionlint (1.7.4)
95-95: label "runner=large" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-22.04", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "macos-12-xl", "macos-12-xlarge", "macos-12-large", "macos-12", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
| - name: Prepare variables | ||
| id: vars | ||
| run: | | ||
| VERSION="${{ needs.prepare.outputs.version }}" | ||
| echo "version=${VERSION}" >> $GITHUB_OUTPUT | ||
| echo "job_name=${JOB_NAME}" >> $GITHUB_OUTPUT | ||
| echo "branch_name=chore/update-${JOB_NAME}-for-${VERSION}" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Undefined JOB_NAME Variable
The variable JOB_NAME is referenced when preparing outputs but is never defined. This will lead to runtime errors. Consider defining it—for example, by setting JOB_NAME="demo"—before using it in the echo statements.
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
|
This PR was closed due to inactivity and merge conflicts. 😭 |
mergify
bot
removed
conflict
what
why
Summary by CodeRabbit
New Features
Chores
.gitignoreentries to prevent tracking of unnecessary files.