Sync repo templates ⚙ - #4468
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is an automated sync from coreos/repo-templates, adding a new ci/shellcheck script for linting shell scripts. The script is generally well-constructed, following good practices. I've identified a couple of areas for improvement related to performance and code clarity. Given that this is a template file, these suggestions should ideally be addressed in the upstream repository to benefit all downstream projects.
|
some golang lint failures. I'll try to take care of them, but it won't be until later. |
8200afd to
64388e8
Compare
dustymabe
changed the title
294eaca to
79d9022
Compare
79d9022 to
9f43f96
Compare
coreosbot-releng
changed the title
6958db1 to
748dfc4
Compare
|
hey @HuijingHei - I'll take this one. I was playing with it last week and got busy but there are a few things that need to happen before I can merge this one. |
In [1] we configured repo-templates to push the go and shellcheck workflows to the COSA repo. Right now the existing golangci-lint workflow here is failing because of golang versions. Let's just drop the existing lints here prematurely knowing that we are going to move to the new repo-templates provided linters in [2]. [1] coreos/repo-templates#359 [2] coreos#4468
In [1] we configured repo-templates to push the go and shellcheck workflows to the COSA repo. Right now the existing golangci-lint workflow here is failing because of golang versions. Let's just drop the existing lints here prematurely knowing that we are going to move to the new repo-templates provided linters in [2]. [1] coreos/repo-templates#359 [2] #4468
748dfc4 to
d678a95
Compare
d678a95 to
44f1167
Compare
44f1167 to
d916160
Compare
d916160 to
f782560
Compare
f782560 to
f025aa4
Compare
f025aa4 to
f9c3768
Compare
f9c3768 to
bf938c8
Compare
📝 WalkthroughWalkthroughAdds Go and ShellCheck GitHub Actions workflows. Go CI checks formatting and linting across two Go versions, while ShellCheck CI detects Bash scripts and runs ShellCheck plus ChangesGo CI
ShellCheck CI
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/go.yml:
- Around line 35-39: Update the Go formatting check in the workflow to avoid
storing and unquoting file lists: use find -exec for path-safe gofmt checks and
diff output, quoting each discovered path and avoiding argument-length limits.
Apply the corresponding change in the upstream workflow template, then
resynchronize this generated workflow file.
- Around line 30-31: Update the actions/checkout step in the workflow to set
persist-credentials to false, ensuring the checkout token is not retained for
subsequent steps. Keep the existing checkout configuration unchanged otherwise.
In @.github/workflows/shellcheck.yml:
- Around line 18-24: Update the actions/checkout step in the workflow to set
persist-credentials to false before executing ci/shellcheck, while preserving
the existing checkout and safe-directory configuration.
In `@ci/shellcheck`:
- Line 9: Update the severity configuration in the ShellCheck gate so
warning-level diagnostics are not ignored; remove the severity restriction or
set it to at least warning. Preserve the existing ShellCheck invocation and
ensure the behavior aligns with tests/check_one.sh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: f7af8173-f3c1-4d40-956a-9c340fd9fcbd
📒 Files selected for processing (3)
.github/workflows/go.yml.github/workflows/shellcheck.ymlci/shellcheck
| - name: Check out repository | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .github/workflows/go.yml
echo '---'
cat -n .github/workflows/go.yml | sed -n '1,120p'Repository: coreos/coreos-assembler
Length of output: 1687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the workflow structure first, then inspect the relevant step bodies.
ast-grep outline .github/workflows/go.yml
echo '---'
sed -n '1,120p' .github/workflows/go.ymlRepository: coreos/coreos-assembler
Length of output: 1379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the workflow uses third-party tooling after checkout and whether
# shell expansions are quoted in the relevant step.
python3 - <<'PY'
from pathlib import Path
text = Path('.github/workflows/go.yml').read_text()
for needle in [
'actions/checkout@v6',
'persist-credentials',
'GO_FILES',
'UNFORMATTED_FILES',
'gofmt',
'third-party',
]:
print(f'[{needle}]', needle in text)
PYRepository: coreos/coreos-assembler
Length of output: 290
🌐 Web query:
actions/checkout v6 persist-credentials default documentation
💡 Result:
The default value for the persist-credentials input in actions/checkout v6 is true [1][2]. When set to true (the default), the action configures the authentication token or SSH key with the local git configuration [1][3]. Starting in v6, this process was updated for improved security to store these credentials in a separate file under $RUNNER_TEMP instead of directly in the.git/config file, although the functional behavior remains the same—allowing subsequent git commands in your workflow to remain authenticated [1][4][2]. You can opt out of this behavior by explicitly setting persist-credentials to false in your workflow file [1][2].
Citations:
- 1: https://github.com/actions/checkout?tab=readme-ov-file
- 2: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 3: https://github.com/actions/checkout/tree/v6.0.0
- 4: https://github.com/actions/checkout
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the workflow section around checkout and the following steps.
nl -ba .github/workflows/go.yml | sed -n '20,80p'Repository: coreos/coreos-assembler
Length of output: 201
🌐 Web query:
site:github.com actions/checkout persist-credentials default true v6
💡 Result:
In the actions/checkout action, including version v6, the default value for the persist-credentials input is true [1][2][3][4]. When this input is set to true (which is the default behavior), the action configures the authentication token or SSH key with the local git configuration, enabling subsequent git commands in your workflow to run with authenticated access [1][3][4]. Starting with v6, the action includes an improvement in credential security: it stores these credentials in a separate file under $RUNNER_TEMP rather than directly within.git/config [1][3][4]. These credentials are automatically removed during post-job cleanup [1][3][4]. If you wish to disable this behavior and prevent the action from configuring credentials, you can explicitly set persist-credentials to false [1][3][4].
Citations:
- 1: https://github.com/actions/checkout?tab=readme-ov-file
- 2: https://github.com/actions/checkout/blob/72f2cec99f417b1a1c5e2e88945068983b7965f9/action.yml
- 3: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 4: https://github.com/actions/checkout/tree/v6.0.0
Disable checkout credential persistence. actions/checkout@v6 leaves the token available to later steps by default; set persist-credentials: false unless a later git operation needs it.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 30-31: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/go.yml around lines 30 - 31, Update the actions/checkout
step in the workflow to set persist-credentials to false, ensuring the checkout
token is not retained for subsequent steps. Keep the existing checkout
configuration unchanged otherwise.
Source: Linters/SAST tools
| GO_FILES=$(find . -name '*.go' -not -path "./vendor/*") | ||
| UNFORMATTED_FILES=$(gofmt -l $GO_FILES) | ||
| if [ -n "$UNFORMATTED_FILES" ]; then | ||
| echo "Go files are not formatted. Please run 'gofmt -w .' on your code." | ||
| gofmt -d $UNFORMATTED_FILES |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the gofmt file handling path-safe.
Both variables are expanded unquoted, so filenames containing whitespace or glob characters are split or expanded incorrectly; passing a large file list can also hit argument-length limits. Use find -exec and quote each path when displaying diffs. Apply this in the upstream template, then resync this file.
Proposed fix
- GO_FILES=$(find . -name '*.go' -not -path "./vendor/*")
- UNFORMATTED_FILES=$(gofmt -l $GO_FILES)
+ UNFORMATTED_FILES=$(find . -name '*.go' -not -path './vendor/*' -exec gofmt -l {} +)
if [ -n "$UNFORMATTED_FILES" ]; then
echo "Go files are not formatted. Please run 'gofmt -w .' on your code."
- gofmt -d $UNFORMATTED_FILES
+ while IFS= read -r file; do
+ gofmt -d "$file"
+ done <<< "$UNFORMATTED_FILES"
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.
| GO_FILES=$(find . -name '*.go' -not -path "./vendor/*") | |
| UNFORMATTED_FILES=$(gofmt -l $GO_FILES) | |
| if [ -n "$UNFORMATTED_FILES" ]; then | |
| echo "Go files are not formatted. Please run 'gofmt -w .' on your code." | |
| gofmt -d $UNFORMATTED_FILES | |
| UNFORMATTED_FILES=$(find . -name '*.go' -not -path './vendor/*' -exec gofmt -l {} +) | |
| if [ -n "$UNFORMATTED_FILES" ]; then | |
| echo "Go files are not formatted. Please run 'gofmt -w .' on your code." | |
| while IFS= read -r file; do | |
| gofmt -d "$file" | |
| done <<< "$UNFORMATTED_FILES" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/go.yml around lines 35 - 39, Update the Go formatting
check in the workflow to avoid storing and unquoting file lists: use find -exec
for path-safe gofmt checks and diff output, quoting each discovered path and
avoiding argument-length limits. Apply the corresponding change in the upstream
workflow template, then resynchronize this generated workflow file.
Source: Linters/SAST tools
| - name: Check out repository | ||
| uses: actions/checkout@v6 | ||
| # https://github.com/actions/checkout/issues/760 | ||
| - name: Mark git checkout as safe | ||
| run: git config --global --add safe.directory "$GITHUB_WORKSPACE" | ||
| - name: Run ShellCheck | ||
| run: ci/shellcheck |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable checkout credential persistence before running PR-controlled code.
The workflow executes ci/shellcheck from the checkout. actions/checkout persists credentials by default, allowing modified PR code to access the checkout token; contents: read still permits repository reads. Add persist-credentials: false. (github.com)
Proposed fix
- name: Check out repository
uses: actions/checkout@v6
+ with:
+ persist-credentials: false📝 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: Check out repository | |
| uses: actions/checkout@v6 | |
| # https://github.com/actions/checkout/issues/760 | |
| - name: Mark git checkout as safe | |
| run: git config --global --add safe.directory "$GITHUB_WORKSPACE" | |
| - name: Run ShellCheck | |
| run: ci/shellcheck | |
| - name: Check out repository | |
| uses: actions/checkout@v6 | |
| with: | |
| persist-credentials: false | |
| # https://github.com/actions/checkout/issues/760 | |
| - name: Mark git checkout as safe | |
| run: git config --global --add safe.directory "$GITHUB_WORKSPACE" | |
| - name: Run ShellCheck | |
| run: ci/shellcheck |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 18-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/shellcheck.yml around lines 18 - 24, Update the
actions/checkout step in the workflow to set persist-credentials to false before
executing ci/shellcheck, while preserving the existing checkout and
safe-directory configuration.
Source: Linters/SAST tools
| main() { | ||
| local found_errors="false" | ||
| # Let's start with error, then we can do warning, info, style | ||
| local -r severity="error" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not restrict the ShellCheck gate to errors only.
Line 9 causes Line 21 to ignore warning-level diagnostics, unlike tests/check_one.sh, which invokes shellcheck -x without a severity restriction. This workflow can therefore pass scripts rejected by the repository’s existing gate. Remove the restriction or use at least warning. (github.com)
Proposed fix
- local -r severity="error"- shellcheck --external-sources --shell bash --severity="${severity}" "${f}" || found_errors="true"
+ shellcheck --external-sources --shell bash "${f}" || found_errors="true"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/shellcheck` at line 9, Update the severity configuration in the ShellCheck
gate so warning-level diagnostics are not ignored; remove the severity
restriction or set it to at least warning. Preserve the existing ShellCheck
invocation and ensure the behavior aligns with tests/check_one.sh.
3 participants
Created by GitHub workflow (source).
Sync with coreos/repo-templates@4bbfd9e.
Summary by CodeRabbit