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 |
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces a new GitHub Actions workflow and various enhancements to the Atmos project, focusing on demo automation and configuration management. Key changes include the addition of a Changes
Suggested Labels
Suggested Reviewers
Possibly Related PRs
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: 0
🧹 Nitpick comments (3)
demo/recordings/demo.tape (3)
2-3: Clarify the commented style configurationThe style configuration is commented out without explanation. Consider either removing the commented line or documenting why it's disabled.
7-10: Consider adjusting sleep durations for better readabilityThe
atmos vendor pullcommand has a 9s sleep while version check has only 1s. Consider standardizing sleep durations based on command complexity:
- Short pauses (500ms) for simple commands
- Medium pauses (1-2s) for command output review
- Longer pauses (5-9s) for complex operations
Also applies to: 19-20
25-37: Document menu navigation purposeThe sequence of Up/Down/Right commands lacks context. Consider adding comments to explain the navigation path and target menu items being selected.
+# Navigate to the components section Down Sleep 500ms Down Sleep 500ms +# Return to main menu Up Sleep 500ms
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
demo/defaults.tape(1 hunks)demo/recordings/demo.tape(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/defaults.tape
🔇 Additional comments (3)
demo/recordings/demo.tape (3)
61-68: Improve content scrolling reliability
The fixed number of Down movements (25x4) might not work consistently with varying content lengths. Consider:
- Using a search pattern to navigate to specific content
- Adding a comment explaining the expected content position
- Using page-based navigation instead of line-based
Would you like me to suggest alternative navigation approaches that are more robust?
1-98: Strong demo script with room for minor improvements!
The demo effectively showcases Atmos capabilities with a logical progression from basic to advanced features. The suggested improvements above would enhance maintainability and reliability, but the core structure is solid.
94-98: Consider making URLs configurable
The documentation and Slack URLs are hardcoded. Consider:
- Moving URLs to a configuration file for easier maintenance
- Adding URL validation to the workflow
✅ Verification successful
URLs are valid and accessible, no immediate action required
Both URLs (https://atmos.tools/ and https://cloudposse.com/slack) return HTTP 200 status codes, confirming they are valid and accessible. While making URLs configurable could be a good future enhancement for maintenance, it's not critical since:
- The URLs are working correctly
- This is a demo recording file, where hardcoded values are often acceptable
- The suggested change would add complexity to what's meant to be a simple demonstration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the URLs are accessible
curl -sL -w "%{http_code}" "https://atmos.tools/" -o /dev/null
curl -sL -w "%{http_code}" "https://cloudposse.com/slack" -o /dev/null
Length of output: 140
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
demo/screengrabs/build-all.sh (2)
10-15:⚠️ Potential issueFix the macOS SED initialization
The macOS branch is incorrectly initializing the SED variable by self-assignment.
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
20-25: 🛠️ Refactor suggestionImprove variable declarations and path handling
Following best practices for shell scripting:
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")"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 22-22: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 25-25: Declare and assign separately to avoid masking return values.
(SC2155)
🧹 Nitpick comments (2)
demo/recordings/style.tape (1)
11-11: Consider documenting playback speed options.The playback speed setting could benefit from a comment explaining available speed options for future maintainers.
Add a comment above the PlaybackSpeed setting:
+# PlaybackSpeed options: 0.5 (slow), 1 (normal), 2 (fast) #Set PlaybackSpeed 1demo/screengrabs/build-all.sh (1)
55-78: Consider enhancing postprocessing robustnessThe postprocessing functions are well-structured, but could benefit from some improvements:
postprocess_ansi() { local file=$1 + [ -f "$file" ] || { echo "Error: File not found: $file"; return 1; } # Process in a single pass for better performance + local patterns=( + '/- Finding latest version of/d' + '/- Installed hashicorp/d' + '/- Installing hashicorp/d' + '/Terraform has created a lock file/d' + '/Include this file in your version control repository/d' + '/guarantee to make the same selections by default when/d' + '/you run "terraform init" in the future/d' + 's/Resource actions are indicated with the following symbols.*//' + '/Workspace .* doesn.t exist./d' + '/You can create this workspace with the .* subcommand/d' + '/or include the .* flag with the .* subcommand./d' + ) + + local expression + printf -v expression ';%s' "${patterns[@]}" + $SED "${expression:1}" "$file" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
demo/recordings/demo.tape(1 hunks)demo/recordings/style.tape(1 hunks)demo/screengrabs/build-all.sh(1 hunks)demo/screengrabs/demo-stacks.txt(1 hunks)demo/screengrabs/scripts/demo-stacks/.demo.rc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- demo/screengrabs/demo-stacks.txt
- demo/recordings/demo.tape
🧰 Additional context used
🪛 Shellcheck (0.10.0)
demo/screengrabs/build-all.sh
[warning] 22-22: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 25-25: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (6)
demo/screengrabs/scripts/demo-stacks/.demo.rc (1)
14-14: Use of --paging=never is beneficial.
This ensures the output is continuously displayed without interruption, improving reliability for demos or automated scripts.
demo/recordings/style.tape (2)
8-9: Verify terminal compatibility for the specified dimensions.
The window dimensions (1400x800) should be tested across different terminal emulators and screen sizes to ensure compatibility.
✅ Verification successful
Terminal dimensions are consistent across demo recordings
The search results show that the dimensions 1400x800 are consistently used across all tape files in the demo directory:
demo/recordings/style.tapedemo/screenshots/sources/style.tapedemo/defaults.tape
This consistency suggests these dimensions have been deliberately chosen as the standard for all demo recordings, making them a proven configuration. The dimensions are defined in defaults.tape, indicating they are part of the default settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for any other dimension configurations in tape files
# to ensure consistency across demos
echo "Searching for dimension settings in other tape files..."
rg -i --type-add 'tape:*.tape' --type tape '(width|height|dimension|size)'
Length of output: 642
6-6: Verify the margin fill color contrast ratio.
The margin fill color "#674EFF" should be tested for accessibility to ensure good contrast with any text or elements that might appear against it.
demo/screengrabs/build-all.sh (3)
5-9: Strong additions for debugging and output control!
The addition of set -x for debug output and LESS=-X for preventing pagination are solid improvements that will help with troubleshooting and automation.
83-85: Solid argument validation!
The argument checking and usage message are well-implemented.
30-44: Well-structured OS-specific command handling!
The OS-specific script command handling is well-implemented, with clear separation between macOS and Linux syntax.
Let's verify the script command options work as expected:
demo/recordings/style.tape
Outdated
| #Set Framerate 10 | ||
| #Set CursorBlink true | ||
| #Set Margin 20 | ||
| #Set MarginFill "#674EFF" |
There was a problem hiding this comment.
Uncomment the VHS settings to make them effective.
The settings are currently commented out with '#', which means they won't take effect. Remove the '#' prefix from each line to activate these configurations.
Apply this diff to fix the issue:
-#Set Framerate 10
-#Set CursorBlink true
-#Set Margin 20
-#Set MarginFill "#674EFF"
+Set Framerate 10
+Set CursorBlink true
+Set Margin 20
+Set MarginFill "#674EFF"📝 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.
| #Set Framerate 10 | |
| #Set CursorBlink true | |
| #Set Margin 20 | |
| #Set MarginFill "#674EFF" | |
| Set Framerate 10 | |
| Set CursorBlink true | |
| Set Margin 20 | |
| Set MarginFill "#674EFF" |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
demo/screengrabs/build-all.sh (2)
8-13:⚠️ Potential issueFix the macOS SED variable initialization
The macOS branch of the conditional is still incorrectly initializing the SED variable by reassigning it 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
18-23: 🛠️ Refactor suggestionImprove variable declarations and path handling
The current implementation could be improved by separating declarations and assignments.
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")"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 23-23: Declare and assign separately to avoid masking return values.
(SC2155)
🧹 Nitpick comments (3)
demo/screengrabs/build-all.sh (3)
28-42: Add path validation before executionThe script changes directories without validating if the paths exist. This could lead to silent failures or unexpected behavior.
Add path validation before executing commands:
+ if [ ! -d "$demo_path" ]; then + echo "Error: Demo path '$demo_path' does not exist" + return 1 + fi + if [ "$(uname)" = "Darwin" ]; then # macOS-specific syntax if [ "${extension}" == "sh" ]; then
53-71: Document the purpose of removed patternsThe postprocess_ansi function removes several patterns without explaining why they're considered noise. Adding comments would help maintainers understand which outputs are intentionally filtered out.
Add documentation for the patterns:
postprocess_ansi() { local file=$1 - # Remove noise and clean up the output + # Remove noise and clean up the output: + # - Remove version check messages $SED '/- Finding latest version of/d' $file + # - Remove Hashicorp installation messages $SED '/- Installed hashicorp/d' $file $SED '/- Installing hashicorp/d' $file + # - Remove Terraform lock file messages $SED '/Terraform has created a lock file/d' $file
Line range hint
81-92: Enhance error handling for manifest processingThe script should validate the manifest file content and handle empty files gracefully.
Add validation:
if [ -z "$manifest" ]; then echo "Usage: $0 <manifest>" exit 1 fi +if [ ! -f "$manifest" ]; then + echo "Error: Manifest file '$manifest' does not exist" + exit 1 +fi + +if [ ! -s "$manifest" ]; then + echo "Error: Manifest file '$manifest' is empty" + exit 1 +fi + demo=$(basename $manifest .txt)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 23-23: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
demo/defaults.tape(1 hunks)demo/recordings/demo.tape(1 hunks)demo/screengrabs/build-all.sh(1 hunks)examples/quick-start-advanced/atmos.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- demo/defaults.tape
- examples/quick-start-advanced/atmos.yaml
- demo/recordings/demo.tape
🧰 Additional context used
🪛 Shellcheck (0.10.0)
demo/screengrabs/build-all.sh
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 23-23: Declare and assign separately to avoid masking return values.
(SC2155)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
demo/recordings/tapes/atmos-help.tape (1)
6-6: Pause duration check
A 3-second sleep is generally fine, but consider using shorter pauses to keep the demo more engaging.demo/recordings/tapes/atmos-terraform-help.tape (1)
4-6: Terraform help demonstration
Displaying native Terraform commands under Atmos control is a nice touch. The 5-second wait is sufficient but slightly long; trimming to 3 seconds might help keep viewers focused.demo/recordings/tapes/atmos-tui.tape (3)
1-2: Incorporate descriptive comments for maintainability.Sourcing two files at the top is fine, but consider adding brief notes describing their purpose for future readers who might be unfamiliar with the style or default settings these tapes provide.
7-18: Consider clarity in navigation steps.Repeated Up and Down steps may feel abrupt for watchers. Adding short typed prompts describing navigation intent could clarify the demonstration.
19-20: Introduce a concluding message.After pressing Enter and pausing, consider concluding with a short typed command or message to signal the demonstration’s completion.
demo/recordings/tapes/atmos-describe-stacks.tape (3)
1-2: Maintain consistent referencing conventions.Sourcing these two tapes is good, but explain in a short note how they affect subsequent commands, similar to the approach in other tape files.
4-5: Expand commentary for demonstration clarity.Consider adding a short typed explanation about the difference between describing a single stack vs. listing multiple stacks. This helps viewers understand the objective behind each command.
12-17: Add input prompts for readability.Scrolling through output with multiple downs can be confusing. Display a short typed message or prompt before using “less” or “q” to improve clarity.
demo/defaults.tape (2)
22-29: Review memory usage constraints.1280x720 is typically safe, but keep a close watch on free-tier memory usage. If any user encounters issues, consider lowering the framerate or scaling down the resolution.
31-31: Shell consistency note.Using bash is standard, but if your environment might default to another shell, ensure the script references align with your shell usage in other files (like build.sh).
demo/recordings/build.sh (3)
19-23: Consider partial cleanup for incremental builds.“clean” deletes all output files. An option to selectively remove certain output (like just GIFs) can be helpful for partial rebuilds.
63-68: Careful fade timing.A 5-second fade at the tail of scenes is presumably correct, but if scene durations are very short, adjust fade timing to avoid overshadowing the entire audio track.
70-79: Check GIF color quality.You generate a palette for each GIF. Consider adjusting the palette generation options to further improve output quality if color banding is noticed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
demo/recordings/LICENSE.pdfis excluded by!**/*.pdfdemo/recordings/background.mp3is excluded by!**/*.mp3
📒 Files selected for processing (20)
.github/workflows/demo.yml(1 hunks)demo/defaults.tape(1 hunks)demo/recordings/README.md(1 hunks)demo/recordings/build.sh(1 hunks)demo/recordings/freeze.json(1 hunks)demo/recordings/scenes/atmos.txt(1 hunks)demo/recordings/style.tape(1 hunks)demo/recordings/tapes/atmos-describe-stacks.tape(1 hunks)demo/recordings/tapes/atmos-docs.tape(1 hunks)demo/recordings/tapes/atmos-help.tape(1 hunks)demo/recordings/tapes/atmos-list-components.tape(1 hunks)demo/recordings/tapes/atmos-list-stacks.tape(1 hunks)demo/recordings/tapes/atmos-outro.tape(1 hunks)demo/recordings/tapes/atmos-terraform-help.tape(1 hunks)demo/recordings/tapes/atmos-tui.tape(1 hunks)demo/recordings/tapes/atmos-validate-stacks.tape(1 hunks)demo/recordings/tapes/atmos-vendor-pull.tape(1 hunks)demo/recordings/tapes/atmos-version.tape(1 hunks)demo/recordings/tapes/atmos-workflows.tape(1 hunks)demo/recordings/tapes/ls-quick-start.tape(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- demo/recordings/README.md
- demo/recordings/tapes/atmos-vendor-pull.tape
- demo/recordings/tapes/atmos-docs.tape
- demo/recordings/freeze.json
- demo/recordings/tapes/atmos-outro.tape
- demo/recordings/scenes/atmos.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/recordings/style.tape
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:5:180: Double quote to prevent globbing and word splitting
(shellcheck)
157-157: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (26)
demo/recordings/tapes/atmos-help.tape (2)
1-2: Great referencing of base tapes
Thanks for cleanly pulling in shared settings from demo/defaults.tape and demo/recordings/style.tape. This makes the tapes more maintainable and consistent.
4-5: Commands look good
The typed message and atmos --help command appear correct. The delay intervals are appropriate to ensure a smooth demonstration.
demo/recordings/tapes/atmos-list-stacks.tape (2)
1-2: Shared resources
Referencing demo/defaults.tape and demo/recordings/style.tape helps maintain a uniform demo environment. Looks good.
4-6: Command demonstration
Using Type "atmos list stacks" with a short delay is an effective way to show stack listing. The final 2-second pause should give enough time for output to settle.
demo/recordings/tapes/atmos-validate-stacks.tape (2)
1-2: Consistent approach
Importing those same source tapes for defaults and style fosters a standard look and feel across all demos.
4-6: Validation flow
This sequence clearly demonstrates how to validate stacks. The 2-second pause allows for proper output capture.
demo/recordings/tapes/atmos-terraform-help.tape (1)
1-2: Same reuse pattern
It’s good to see the same references again, ensuring consistent theming for these Terraform demos.
demo/recordings/tapes/atmos-version.tape (4)
1-2: Seems straightforward.
Sourcing the default and style tapes is a fine approach for consistency across demos.
4-4: Sleep command is good for pacing.
The short delay helps avoid missed commands during interactive recordings.
5-6: Command sequence is concise and clear.
Displaying a helpful comment before running "atmos version" improves clarity for end-users.
7-7: Sufficient wait time for version check.
A 2-second pause is a nice buffer for processes to complete.
demo/recordings/tapes/atmos-list-components.tape (2)
1-2: Good reuse of defaults and style tapes.
This keeps the demos consistent and maintainable.
4-6: Clear demonstration flow.
Providing a brief explanatory message before listing Atmos components makes the script more instructive.
demo/recordings/tapes/ls-quick-start.tape (3)
1-2: Consistent setup across tapes.
Sourcing default and style tapes ensures a uniform baseline for all demos.
4-7: Terminal visibility toggling is neat.
Hiding the shell commands, clearing, and then showing offers a clean transition for watchers.
9-13: Commands are self-explanatory.
Guiding users to the quick-start directory and listing files is straightforward and helps them follow the structure.
demo/recordings/tapes/atmos-workflows.tape (2)
1-2: Proper sourcing approach.
Following the same pattern ensures the workflow demonstrations remain consistent.
4-16: Good navigation demonstration.
The simulated key presses and sleeps highlight how to explore workflows step by step. This should be helpful for new users learning Atmos.
demo/recordings/tapes/atmos-tui.tape (1)
4-5: Check for potential duplication.
The typed comment in line 4 references exploring components, while line 5 proceeds to run “atmos.” Ensure you’re not duplicating the same user context in other tapes.
demo/defaults.tape (2)
5-6: Confirm theme color choices.
The chosen color palette is quite vibrant. Ensure it doesn’t hinder readability for certain background/foreground color pairings.
8-10: Verify font compatibility across systems.
Since “Hack Nerd Font” is commented out, confirm “FiraCode Nerd Font” is installed on all relevant environments or specify a fallback to avoid rendering issues.
.github/workflows/demo.yml (5)
3-9: Add demo-related paths to workflow triggers.
The workflow should also trigger when demo-related files are modified to ensure demos stay in sync with content changes.
Add these paths to the trigger configuration:
paths:
- ".github/workflows/demo.yml"
+ - "demo/**"
+ - "docs/demo.gif"
+ - "website/static/img/demo.gif"23-27: Enhance version retrieval with proper error handling.
The curl command needs error handling and proper output validation.
run: |
- VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
- echo "version=$VERSION" >> $GITHUB_OUTPUT
+ if ! VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name); then
+ echo "Failed to fetch latest release"
+ exit 1
+ fi
+ if [ -z "$VERSION" ]; then
+ echo "Version is empty"
+ exit 1
+ fi
+ echo "version=$VERSION" >> "$GITHUB_OUTPUT"🧰 Tools
🪛 actionlint (1.7.4)
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
37-40: Add error handling to package installation.
The dependency installation step should verify that all required packages are installed successfully.
run: |
- sudo apt-get update
- sudo apt-get install -y aha util-linux make jq bat
- sudo ln -s /usr/bin/batcat /usr/bin/bat
+ if ! sudo apt-get update; then
+ echo "Failed to update package list"
+ exit 1
+ fi
+ if ! sudo apt-get install -y aha util-linux make jq bat; then
+ echo "Failed to install dependencies"
+ exit 1
+ fi
+ if ! sudo ln -sf /usr/bin/batcat /usr/bin/bat; then
+ echo "Failed to create bat symlink"
+ exit 1
+ fi87-90: Pin the set-swap-space action to a specific version.
Using a commit SHA for the action version is not recommended. Use a proper version tag instead.
-uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c
+uses: pierotofy/[email protected]148-153: Add proper quoting to GitHub step summary commands.
The GITHUB_STEP_SUMMARY environment variable needs proper quoting to prevent word splitting.
-echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY
-echo "" >> $GITHUB_STEP_SUMMARY
-echo "## Demo Video" >> $GITHUB_STEP_SUMMARY
-echo "" >> $GITHUB_STEP_SUMMARY
+{
+ echo "## Demo GIF"
+ echo ""
+ echo "## Demo Video"
+ echo ""
+} >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
148-148: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:5:180: Double quote to prevent globbing and word splitting
(shellcheck)
| REPO_ROOT="$(git rev-parse --show-toplevel)" | ||
| TAPES_DIR="$REPO_ROOT/demo/recordings/tapes" | ||
| SCENES_DIR="$REPO_ROOT/demo/recordings/scenes" | ||
| MP4_OUTDIR="$REPO_ROOT/demo/recordings/mp4" | ||
| GIF_OUTDIR="$REPO_ROOT/demo/recordings/gif" | ||
| AUDIO_FILE="$REPO_ROOT/demo/recordings/background.mp3" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing dependencies gracefully.
You rely on git, vhs, and ffmpeg. Consider adding checks to confirm these tools are installed, providing clearer error messaging if missing.
| base="$(basename "$tape" .tape)" | ||
| echo " Processing $tape -> $MP4_OUTDIR/$base.mp4" | ||
| (cd "$REPO_ROOT" && vhs "$tape" --output "$MP4_OUTDIR/$base.mp4") | ||
| done |
There was a problem hiding this comment.
Add error handling around the vhs command.
If the vhs invocation fails, consider exiting early with an informative message to prevent subsequent steps from running on partial output.
| - name: Record screencast | ||
| run: | | ||
| demo/recordings/build.sh | ||
| mv demo/recordings/gif/atmos.gif docs/demo.gif |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to screencast recording.
The recording step needs proper error handling and validation.
run: |
- demo/recordings/build.sh
- mv demo/recordings/gif/atmos.gif docs/demo.gif
+ if ! demo/recordings/build.sh; then
+ echo "Failed to build screencast"
+ exit 1
+ fi
+ if ! mv demo/recordings/gif/atmos.gif docs/demo.gif; then
+ echo "Failed to move demo gif"
+ 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: Record screencast | |
| run: | | |
| demo/recordings/build.sh | |
| mv demo/recordings/gif/atmos.gif docs/demo.gif | |
| - name: Record screencast | |
| run: | | |
| if ! demo/recordings/build.sh; then | |
| echo "Failed to build screencast" | |
| exit 1 | |
| fi | |
| if ! mv demo/recordings/gif/atmos.gif docs/demo.gif; then | |
| echo "Failed to move demo gif" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/demo.yml (1)
148-153: Fix script quoting in job summary steps!run: | - echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - - echo "## Demo Video" >> $GITHUB_STEP_SUMMARY - echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo GIF" + echo "" + echo + echo "## Demo Video" + echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" + } >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
148-148: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)demo/recordings/build.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- demo/recordings/build.sh
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
114-114: shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
148-148: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
157-157: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/demo.yml (5)
87-90: 🛠️ Refactor suggestion
Pin the set-swap-space action to a specific version!
Using a commit SHA is not recommended. Use a proper version tag:
- uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c
+ uses: pierotofy/[email protected]Likely invalid or redundant comment.
25-27: 🛠️ Refactor suggestion
Add error handling to version retrieval!
The curl command needs error handling and proper output validation:
run: |
- VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name)
- echo "version=$VERSION" >> $GITHUB_OUTPUT
+ if ! VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name); then
+ echo "Failed to fetch latest release"
+ exit 1
+ fi
+ if [ -z "$VERSION" ]; then
+ echo "Version is empty"
+ exit 1
+ fi
+ echo "version=$VERSION" >> "$GITHUB_OUTPUT"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
126-129: 🛠️ Refactor suggestion
Add error handling to screencast recording!
run: |
- demo/recordings/build.sh
- mv demo/recordings/gif/atmos.gif docs/demo.gif
+ if ! demo/recordings/build.sh; then
+ echo "Failed to build screencast"
+ exit 1
+ fi
+ if ! mv demo/recordings/gif/atmos.gif docs/demo.gif; then
+ echo "Failed to move demo gif"
+ exit 1
+ fiLikely invalid or redundant comment.
3-9: 🛠️ Refactor suggestion
Expand workflow trigger paths to include demo-related files!
The workflow should also trigger when demo content changes. Add these paths:
on:
workflow_dispatch:
pull_request:
types: [labeled, opened, synchronize]
paths:
- ".github/workflows/demo.yml"
+ - "demo/**"
+ - "docs/demo.gif"
+ - "website/static/img/demo.gif"Likely invalid or redundant comment.
37-40: 🛠️ Refactor suggestion
Add error handling to package installation!
run: |
- sudo apt-get update
- sudo apt-get install -y aha util-linux make jq bat
- sudo ln -s /usr/bin/batcat /usr/bin/bat
+ if ! sudo apt-get update; then
+ echo "Failed to update package list"
+ exit 1
+ fi
+ if ! sudo apt-get install -y aha util-linux make jq bat; then
+ echo "Failed to install dependencies"
+ exit 1
+ fi
+ if ! sudo ln -sf /usr/bin/batcat /usr/bin/bat; then
+ echo "Failed to create bat symlink"
+ exit 1
+ fiLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/demo.yml (2)
97-100: Pin the set-swap-space action to a specific version.Using a commit SHA is not recommended. Use a proper version tag.
- uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c + uses: pierotofy/[email protected]
158-163: Add proper quoting to GitHub step summary commands.The GITHUB_STEP_SUMMARY environment variable needs proper quoting.
run: | - echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY - - echo "## Demo Video" >> $GITHUB_STEP_SUMMARY - echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo GIF" + echo "" + echo + echo "## Demo Video" + echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" + } >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
158-158: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/demo.yml(1 hunks)demo/recordings/.gitignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- demo/recordings/.gitignore
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/demo.yml
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
124-124: shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
158-158: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
167-167: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/demo.yml (5)
136-139: 🛠️ Refactor suggestion
Add error handling to screencast recording.
The recording step needs proper error handling and validation.
run: |
- demo/recordings/build.sh
- mv demo/recordings/gif/atmos.gif docs/demo.gif
+ if ! demo/recordings/build.sh; then
+ echo "Failed to build screencast"
+ exit 1
+ fi
+ if ! mv demo/recordings/gif/atmos.gif docs/demo.gif; then
+ echo "Failed to move demo gif"
+ exit 1
+ fiLikely invalid or redundant comment.
23-27: 🛠️ Refactor suggestion
Add error handling to version retrieval.
The curl command needs proper error handling and output validation.
- 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
+ if ! VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name); then
+ echo "Failed to fetch latest release"
+ exit 1
+ fi
+ if [ -z "$VERSION" ]; then
+ echo "Version is empty"
+ exit 1
+ fi
+ echo "version=$VERSION" >> "$GITHUB_OUTPUT"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
3-8: 🛠️ Refactor suggestion
Add demo-related paths to workflow triggers.
The workflow should also trigger when demo content changes. Add these paths:
on:
workflow_dispatch:
pull_request:
types: [labeled, opened, synchronize]
paths:
- ".github/workflows/demo.yml"
+ - "demo/**"
+ - "docs/demo.gif"Likely invalid or redundant comment.
37-40: 🛠️ Refactor suggestion
Add error handling to package installation.
The dependency installation step should verify successful installation.
run: |
- sudo apt-get update
- sudo apt-get install -y aha util-linux make jq bat
- sudo ln -s /usr/bin/batcat /usr/bin/bat
+ if ! sudo apt-get update; then
+ echo "Failed to update package list"
+ exit 1
+ fi
+ if ! sudo apt-get install -y aha util-linux make jq bat; then
+ echo "Failed to install dependencies"
+ exit 1
+ fi
+ if ! sudo ln -sf /usr/bin/batcat /usr/bin/bat; then
+ echo "Failed to create bat symlink"
+ exit 1
+ fiLikely invalid or redundant comment.
59-63: 🛠️ Refactor suggestion
Add build verification step.
The build step should verify its success before proceeding.
run: |
- make -C demo/screengrabs build-all install
+ if ! make -C demo/screengrabs build-all install; then
+ echo "Build failed"
+ exit 1
+ fi
git add -A
git statusLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/demo.yml (2)
98-101: Pin the set-swap-space action to a specific version.Using a commit SHA for the action version is not recommended. Use a proper version tag instead.
- uses: pierotofy/set-swap-space@0404882bc4666c0ff2f6fd8b3d32af69a730183c + uses: pierotofy/[email protected]
159-164: Add proper quoting to GitHub step summary commands.The GITHUB_STEP_SUMMARY environment variable needs proper quoting.
- echo "## Demo GIF" >> $GITHUB_STEP_SUMMARY - echo "" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo GIF" + echo "" + } >> "$GITHUB_STEP_SUMMARY" - echo "## Demo Video" >> $GITHUB_STEP_SUMMARY - echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" >> $GITHUB_STEP_SUMMARY + { + echo "## Demo Video" + echo "https://github.com/${{ github.repository }}/blob/${{ steps.auto-commit.outputs.pull-request-head-sha }}/demo/recordings/mp4/atmos-with-audio.mp4?raw=true" + } >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
159-159: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
📜 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
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
125-125: shellcheck reported issue in this script: SC2129:style:2:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
125-125: shellcheck reported issue in this script: SC2086:info:2:30: Double quote to prevent globbing and word splitting
(shellcheck)
125-125: shellcheck reported issue in this script: SC2086:info:3:32: Double quote to prevent globbing and word splitting
(shellcheck)
125-125: shellcheck reported issue in this script: SC2086:info:4:63: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:1:23: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:2:151: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:4:25: Double quote to prevent globbing and word splitting
(shellcheck)
159-159: shellcheck reported issue in this script: SC2086:info:5:165: Double quote to prevent globbing and word splitting
(shellcheck)
168-168: shellcheck reported issue in this script: SC2086:info:1:30: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (6)
.github/workflows/demo.yml (6)
137-140: 🛠️ Refactor suggestion
Add error handling to screencast recording.
The recording step needs proper error handling and validation.
run: |
- demo/recordings/build.sh
- mv demo/recordings/gif/atmos.gif docs/demo.gif
+ if ! demo/recordings/build.sh; then
+ echo "Failed to build screencast"
+ exit 1
+ fi
+ if ! mv demo/recordings/gif/atmos.gif docs/demo.gif; then
+ echo "Failed to move demo gif"
+ exit 1
+ fiLikely invalid or redundant comment.
3-9: 🛠️ Refactor suggestion
Expand workflow triggers to include demo-related paths.
The workflow should also trigger when demo content changes. Add these paths to ensure the workflow runs appropriately:
on:
workflow_dispatch:
pull_request:
types: [labeled, opened, synchronize]
paths:
- ".github/workflows/demo.yml"
+ - "demo/**"
+ - "docs/demo.gif"
+ - "website/static/img/demo.gif"Likely invalid or redundant comment.
59-64: 🛠️ Refactor suggestion
Add build verification step.
The build step should verify its success before proceeding.
run: |
- make -C demo/screengrabs build-all install
+ if ! make -C demo/screengrabs build-all install; then
+ echo "Build failed"
+ exit 1
+ fi
git add -A
git statusLikely invalid or redundant comment.
117-122: 🛠️ Refactor suggestion
Enhance Atmos testing with validation.
The current test step only verifies command existence. Add output validation:
- name: Test Atmos
run: |
which atmos
- atmos version
- atmos --help
+ if ! version_output=$(atmos version); then
+ echo "Failed to get Atmos version"
+ exit 1
+ fi
+ if ! echo "$version_output" | grep -q "${{ needs.prepare.outputs.version }}"; then
+ echo "Installed version doesn't match expected version"
+ exit 1
+ fi
+ if ! atmos --help | grep -q "Usage:"; then
+ echo "Help command failed to show usage"
+ exit 1
+ fiLikely invalid or redundant comment.
37-40: 🛠️ Refactor suggestion
Add error handling to package installation.
The dependency installation step should verify that all required packages are installed successfully.
run: |
- sudo apt-get update
- sudo apt-get install -y aha util-linux make jq bat
- sudo ln -s /usr/bin/batcat /usr/bin/bat
+ if ! sudo apt-get update; then
+ echo "Failed to update package list"
+ exit 1
+ fi
+ if ! sudo apt-get install -y aha util-linux make jq bat; then
+ echo "Failed to install dependencies"
+ exit 1
+ fi
+ if ! sudo ln -sf /usr/bin/batcat /usr/bin/bat; then
+ echo "Failed to create bat symlink"
+ exit 1
+ fiLikely invalid or redundant comment.
23-27: 🛠️ Refactor suggestion
Add error handling to version retrieval.
The curl command needs proper error handling and output validation.
- 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
+ if ! VERSION=$(curl -s --fail https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name); then
+ echo "Failed to fetch latest release"
+ exit 1
+ fi
+ if [ -z "$VERSION" ]; then
+ echo "Version is empty"
+ exit 1
+ fi
+ echo "version=$VERSION" >> "$GITHUB_OUTPUT"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
25-25: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
what
why
Summary by CodeRabbit
New Features
Documentation
atmos.yaml.Chores
.gitignoreentry for background audio file.Bug Fixes