Save a list of observed images after workflow - #1089
Conversation
|
Warning Rate limit exceededAndrei Kvapil (@kvaps) has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 47 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 (3)
""" WalkthroughThis change adds new steps to the GitHub Actions cleanup job to list and upload container images used by sandbox nodes before teardown. It introduces a shell script that collects image information from nodes, modifies the Makefile to support running this script inside the sandbox container, and adds a new Makefile target for listing images. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Makefile
participant Sandbox Container
participant list-images.sh
GitHub Actions->>Makefile: make list-images SANDBOX_NAME
Makefile->>Sandbox Container: Run hack/list-images.sh (mounted /workspace/hosttmp)
Sandbox Container->>list-images.sh: Execute script to gather images
list-images.sh->>Sandbox Container: Write images.txt in shared directory
Sandbox Container->>Makefile: Script completes
GitHub Actions->>Makefile: make delete SANDBOX_NAME
Makefile->>Sandbox Container: Tear down sandbox environment
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/core/testing/Makefile (1)
52-60: Clean up mount directory on delete and tighten permissionsAvoid leaving behind stale directories and overly permissive modes. Remove
/tmp/${SANDBOX_NAME}in thedeletetarget and change the mount folder’s permissions from777to700:delete: ## Remove sandbox from existing Kubernetes cluster. - docker rm -f "${SANDBOX_NAME}" || true + docker rm -f "${SANDBOX_NAME}" || true + rm -rf /tmp/${SANDBOX_NAME} || true apply: delete - mkdir -p /tmp/${SANDBOX_NAME} - chmod 777 /tmp/${SANDBOX_NAME} + mkdir -p /tmp/${SANDBOX_NAME} + chmod 700 /tmp/${SANDBOX_NAME} docker run \ -d --rm --name "${SANDBOX_NAME}" --privileged \ -e TALOSCONFIG=/workspace/talosconfig \ -e KUBECONFIG=/workspace/kubeconfig \ -e SANDBOX_NAME=${SANDBOX_NAME} \ -v /tmp/${SANDBOX_NAME}:/workspace/hosttmp \ "$$(yq .e2e.image values.yaml)" \ sleep infinity
🧹 Nitpick comments (2)
hack/list-images.sh (1)
5-8: Improve readability with node headers and quote variablesPrepend a header for each node’s images block and quote the IP expansions to guard against unexpected splitting or globbing.
-for node in 11 12 13; do - talosctl -n 192.168.123.${node} -e 192.168.123.${node} images ls >> ./hosttmp/images.txt - talosctl -n 192.168.123.${node} -e 192.168.123.${node} images --namespace system ls >> ./hosttmp/images.txt +for node in 11 12 13; do + echo "=== Images for node 192.168.123.${node} ===" >> ./hosttmp/images.txt + talosctl -n "192.168.123.${node}" -e "192.168.123.${node}" images ls >> ./hosttmp/images.txt + talosctl -n "192.168.123.${node}" -e "192.168.123.${node}" images --namespace system ls >> ./hosttmp/images.txt donepackages/core/testing/Makefile (1)
70-72: Document the newlist-imagestargetAdd a help docstring so it appears in
make help:-list-images: +list-images: ## List Talos node images into shared `/workspace/hosttmp` docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && hack/list-images.sh'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests.yaml(1 hunks)hack/list-images.sh(1 hunks)packages/core/testing/Makefile(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/pull-requests.yaml
157-157: shellcheck reported issue in this script: SC2086:info:1:44: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
156-160: ```shell
#!/bin/bashExtract the relevant section from the workflow to verify current configuration
sed -n '140,180p' .github/workflows/pull-requests.yaml
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| #!/bin/sh | ||
|
|
||
| rm ./hosttmp/images.txt | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add strict mode and safe file removal
Enable pipefail and exit-on-error/undefined, and switch to rm -f so the script doesn’t error if images.txt is missing. Also ensure the hosttmp directory exists.
-#!/bin/sh
+#!/bin/sh
+set -euo pipefail
-mkdir -p ./hosttmp
-rm ./hosttmp/images.txt
+mkdir -p ./hosttmp
+rm -f ./hosttmp/images.txtCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In hack/list-images.sh at lines 1 to 4, add strict mode by enabling 'set -euo
pipefail' at the top of the script to ensure it exits on errors or undefined
variables and fails on pipeline errors. Change the 'rm ./hosttmp/images.txt'
command to 'rm -f ./hosttmp/images.txt' to avoid errors if the file does not
exist. Before removing the file, add a check or command to ensure the 'hosttmp'
directory exists to prevent errors related to missing directories.
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Please make artifact apploaded to the github releases as images.list
Prefer using docker cp instead of -v, as it is not working with remote docker daemon
| for node in 11 12 13; do | ||
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images ls >> ./hosttmp/images-${SANDBOX_NAME}.txt | ||
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images --namespace system ls >> ./hosttmp/images-${SANDBOX_NAME}.txt | ||
| done |
There was a problem hiding this comment.
| for node in 11 12 13; do | |
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images ls >> ./hosttmp/images-${SANDBOX_NAME}.txt | |
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images --namespace system ls >> ./hosttmp/images-${SANDBOX_NAME}.txt | |
| done | |
| for node in 11 12 13; do | |
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images ls | awk '{print $2}' >> ./hosttmp/images-${SANDBOX_NAME}.txt.tmp | |
| talosctl -n 192.168.123.${node} -e 192.168.123.${node} images --namespace system ls | awk '{print $2}' >> ./hosttmp/images-${SANDBOX_NAME}.txt.tmp | |
| done | |
| sort -u ./hosttmp/images-${SANDBOX_NAME}.txt.tmp > ./hosttmp/images-${SANDBOX_NAME}.txt | |
| rm -f ./hosttmp/images-${SANDBOX_NAME}.txt.tmp |
aa71b78 to
e3614b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
195-197: Quote the variable to satisfy shell-lint & avoid edge-case word-splittingShellCheck (see previous lints) still complains about
SC2086here.
Although$SANDBOX_NAMEis unlikely to contain spaces, quoting is free and silences the linter.- run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME list-images + run: make -C packages/core/testing SANDBOX_NAME="$SANDBOX_NAME" list-images
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests.yaml(1 hunks)hack/list-images.sh(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- hack/list-images.sh
- packages/core/testing/Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 203-203: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
198-203: Artifact may be empty – path & resiliency need adjustment
images.txtis referenced relative to the job’s work-dir, butlist-images
executes inside the sandbox container via Make. Unless that target explicitly copies
the file back to${{ github.workspace }}, the upload will silently create an empty artifact.
Typical location would be/tmp/$SANDBOX_NAME/images.txt.The upload step should run regardless of earlier failures so that we don’t lose diagnostics.
Line 203 has trailing whitespace ‑ YAML-lint error.
Proposed patch:
- - name: Upload image list - uses: actions/upload-artifact@v4 - with: - name: image-list - path: images.txt + - name: Upload image list + if: always() + uses: actions/upload-artifact@v4 + with: + name: image-list + path: /tmp/$SANDBOX_NAME/images.txt
e3614b5 to
d1e61aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-requests.yaml (1)
195-197: Quote the$SANDBOX_NAMEvariable to satisfy ShellCheck SC2086Previous Action Lint feedback (SC2086) is still applicable.
The un-quoted variable can expand to multiple words and breakmake.- run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME list-images + run: make -C packages/core/testing SANDBOX_NAME="$SANDBOX_NAME" list-images
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-requests.yaml(1 hunks)hack/list-images.sh(1 hunks)packages/core/testing/Makefile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- hack/list-images.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/testing/Makefile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pull-requests.yaml
[error] 203-203: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
.github/workflows/pull-requests.yaml (1)
198-203: ```bash
#!/bin/bash
set -eecho "== Workflow snippet around the upload step (lines 150–250) =="
sed -n '150,250p' .github/workflows/pull-requests.yamlecho -e "\n== Steps referencing 'list-images' in the workflow =="
grep -R "list-images" -n .github/workflows/pull-requests.yamlecho -e "\n== References to 'images.txt' across the repo =="
grep -R "images.txt" -n .</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
d1e61aa to
529d7a3
Compare
529d7a3 to
6b2cd0d
Compare
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Because the sandbox is torn down after successful tests Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
09ddf7c to
d59ec5b
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
d59ec5b to
8ec882c
Compare
Summary by CodeRabbit