chore: Screengrabs #1239
chore: Screengrabs #1239
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 |
milldr
added
the
no-release
📝 Walkthrough""" WalkthroughA new GitHub Actions workflow named "Screengrabs" was added to automate fetching the latest Atmos release, building screengrabs, and creating or updating a pull request with the changes. Minor updates were also made to demo screengrab scripts and the Makefile to improve cross-platform compatibility and output handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant GitHub API
participant Make
participant Git
participant PR
User->>GitHub Actions: Trigger "Screengrabs" workflow (workflow_dispatch)
GitHub Actions->>GitHub API: Fetch latest Atmos release version
GitHub Actions->>Make: Build and install screengrabs (from demo directory)
Make->>Git: Stage all changes
GitHub Actions->>PR: Create or update PR with screengrab changes
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 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 (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (3)
.github/workflows/screengrabs.yaml (3)
20-23: Enhance error handling in version fetchIf the GitHub API call fails, you’ll end up with an empty
VERSIONand the workflow will continue. Addset -euo pipefailto catch failures early:run: | - VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) + set -euo pipefail + VERSION=$(curl -s https://api.github.com/repos/cloudposse/atmos/releases/latest | jq -r .tag_name) echo "version=$VERSION" >> $GITHUB_OUTPUT
33-36: Optimize dependency installationCombine update and install, minimize image size, and disable prompts:
-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 +run: | + sudo DEBIAN_FRONTEND=noninteractive apt-get update && \ + sudo DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends aha util-linux make jq bat && \ + sudo ln -sf /usr/bin/batcat /usr/bin/bat
37-38: Clarify step name for Windows line endingsTypographical fix: capitalize “Windows” and clarify intent:
-name: Set Git Preferences for windows +name: Set Git preferences for Windows line endings
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/screengrabs.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/screengrabs.yaml
2-2: unexpected key "description" for "workflow" section. expected one of "concurrency", "defaults", "env", "jobs", "name", "on", "permissions", "run-name"
(syntax-check)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1239 +/- ##
=======================================
Coverage 33.56% 33.56%
=======================================
Files 226 226
Lines 24219 24219
=======================================
Hits 8130 8130
Misses 14875 14875
Partials 1214 1214
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
demo/screengrabs/build-all.sh (5)
20-23: Avoid masking return codes in local assignments
ShellCheck SC2155 warns aboutlocal var=$(cmd)masking the exit status ofcmd. Consider splitting declaration and assignment, and quote variables to prevent word-splitting:-local output_base_file=artifacts/$(echo "$command" | …) +local output_base_file +output_base_file=$(echo "$command" | …) -local output_dir=$(dirname $output_base_file) +local output_dir +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)
26-27: Quote variables when cleaning up
Unquoted$output_ansimay break if the path ever contains spaces. For safety, wrap it in quotes:-rm -f $output_ansi +rm -f "$output_ansi"
28-42: DRY up OS checks and script invocation
The nestedunamecalls and repetition ofscriptlogic can be simplified by computing the OS once and abstracting common flags:OS=$(uname) if [ "$OS" = "Darwin" ]; then SCRIPT_BASE=(script -q) else SCRIPT_BASE=(script -q -a) fi # later... "${SCRIPT_BASE[@]}" "$output_ansi" -c "$command" > /dev/nullThis reduces duplication and makes maintenance easier.
55-71: Combine sed invocations for readability and performance
You’re calling$SEDonce per pattern. Consider merging them with multiple-eflags (or a heredoc script) to reduce process overhead:-postprocess_ansi() { - $SED '/- Finding latest version of/d' $file - $SED '/- Installed hashicorp/d' $file - … (many lines) +postprocess_ansi() { + $SED -e '/- Finding latest version of/d' \ + -e '/- Installed hashicorp/d' \ + -e '/- Installing hashicorp/d' \ + -e '/Terraform has created a lock file/d' \ + … \ + "$file"This keeps the function concise and faster.
83-84: Write usage message to stderr
Usage prompts should go to stderr in CLI tools so they don’t pollute stdout streams:- echo "Usage: $0 <manifest>" + echo "Usage: $0 <manifest>" >&2 exit 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
demo/screengrabs/Makefile(1 hunks)demo/screengrabs/build-all.sh(1 hunks)demo/screengrabs/scripts/demo-stacks/.demo.rc(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- demo/screengrabs/Makefile
- demo/screengrabs/scripts/demo-stacks/.demo.rc
🧰 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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Analyze (go)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (4)
demo/screengrabs/build-all.sh (4)
5-6: Disable pagination for consistent output
ExportingLESS=-Xis a solid move to prevent any pager from interrupting the capture process on both macOS and Linux.
16-19: Scope variables locally inrecord
Good use oflocalfordemo,command,extension, anddemo_pathto avoid polluting the global namespace.
43-46: Post-processing and cleanup look solid
Callingpostprocess_ansi, converting withaha, thenpostprocess_htmlfollowed by removing the ANSI file is clear and effective.
75-77: HTML postprocessing updates look correct
The color substitutions (blue→#005f87,#183691→#005f87) align with your branding update. Nice and straightforward.
| # 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 macOS sed in-place syntax
The assignment SED="$SED" is a no-op and won’t invoke sed correctly on Darwin. You need to include the empty string argument for BSD sed’s -i flag.
Apply this patch:
-if [ "$(uname)" = "Darwin" ]; then
- SED="$SED" # macOS requires '' for in-place editing
+if [ "$(uname)" = "Darwin" ]; then
+ SED="sed -i ''" # BSD sed: in-place requires an explicit empty suffix
else
- SED="sed -i" # Linux does not require ''
+ SED="sed -i" # GNU sed: in-place works without suffix
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 ''" # BSD sed: in-place requires an explicit empty suffix | |
| else | |
| SED="sed -i" # GNU sed: in-place works without suffix | |
| fi |
🤖 Prompt for AI Agents (early access)
In demo/screengrabs/build-all.sh around lines 8 to 13, the macOS sed in-place
editing syntax is incorrect because the assignment SED="$SED" does nothing. Fix
this by setting SED to "sed -i ''" for Darwin to correctly use BSD sed's
in-place editing with an empty string argument, while keeping the Linux case as
"sed -i".
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
what
why
references
Summary by CodeRabbit