Fix problems with GitHub actions #1691
Conversation
|
Warning Rate limit exceeded@alexeyr-ci has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 49 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)
WalkthroughThe pull request introduces updates to GitHub Actions workflow files, focusing on modernizing the syntax for setting output variables and enhancing workflow configurations. The changes primarily involve replacing the deprecated Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/lint-js-and-ruby.yml (1)
68-75: LGTM! Consider caching the Actionlint binary.The Actionlint integration is well implemented, with proper conditional execution. However, downloading Actionlint on every workflow run could be optimized.
Consider caching the Actionlint binary using GitHub's cache action:
- name: Lint GitHub Actions # We only download and run Actionlint if there is any difference in GitHub Action workflows + id: check-workflows run: | if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q '^.github/workflows'; then + echo "changed=true" >> $GITHUB_OUTPUT + fi + shell: bash + - name: Setup Actionlint + if: steps.check-workflows.outputs.changed == 'true' + uses: actions/cache@v4 + id: cache-actionlint + with: + path: ./actionlint + key: ${{ runner.os }}-actionlint-${{ hashFiles('.github/workflows/**') }} + - name: Download Actionlint + if: steps.check-workflows.outputs.changed == 'true' && steps.cache-actionlint.outputs.cache-hit != 'true' + run: bash <(curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash) + - name: Run Actionlint + if: steps.check-workflows.outputs.changed == 'true' + run: ./actionlint -color
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/examples.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(3 hunks).travis.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .travis.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/examples.yml
64-64: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/main.yml
76-76: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
136-136: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/main.yml (1)
161-161: LGTM! Correct fix for version comparison.The change from
<to-ltproperly fixes the string comparison to numeric comparison for Chrome versions.
.github/workflows/examples.yml
Outdated
| @@ -61,7 +61,7 @@ jobs: | |||
| path: vendor/bundle | |||
| key: package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | |||
| - id: get-sha | |||
| run: echo "::set-output name=sha::$(git rev-parse HEAD)" | |||
| run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote the command substitution in the echo statement.
While the migration from ::set-output to $GITHUB_OUTPUT is correct, the command substitution should be quoted to prevent word splitting and globbing.
Apply this fix:
- run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
+ run: echo "sha=\"$(git rev-parse HEAD)\"" >> $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.
| run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT | |
| run: echo "sha=\"$(git rev-parse HEAD)\"" >> $GITHUB_OUTPUT |
🧰 Tools
🪛 actionlint (1.7.4)
64-64: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/main.yml
Outdated
| @@ -73,7 +73,7 @@ jobs: | |||
| - name: Build test bundles for dummy app | |||
| run: cd spec/dummy && rm -rf public/webpack/test && yarn run build:rescript && RAILS_ENV=test NODE_ENV=test bin/${{ matrix.versions == 'oldest' && 'web' || 'shaka' }}packer | |||
| - id: get-sha | |||
| run: echo "::set-output name=sha::$(git rev-parse HEAD)" | |||
| run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote the command substitution in both echo statements.
Similar to examples.yml, the command substitutions should be quoted in both SHA output steps.
Apply this fix to both locations:
- run: echo "sha=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
+ run: echo "sha=\"$(git rev-parse HEAD)\"" >> $GITHUB_OUTPUTAlso applies to: 136-136
🧰 Tools
🪛 actionlint (1.7.4)
76-76: shellcheck reported issue in this script: SC2086:info:1:37: Double quote to prevent globbing and word splitting
(shellcheck)
alexeyr-ci
force-pushed
the
alexeyr/ci-workflow-fix
branch
4 times, most recently
from
c41cbc6 to
5bf6e8e
Compare
| @@ -1,338 +0,0 @@ | |||
| # CircleCI is not longer in use by this repository & this file is now for reference only. | |||
There was a problem hiding this comment.
I think even if we ever want to switch back, the starting point should be actually maintained GitHub Actions workflows instead of this.
alexeyr-ci
force-pushed
the
alexeyr/ci-workflow-fix
branch
from
5bf6e8e to
5e5c6e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/actionlint-matcher.json (1)
7-7: Consider adding documentation for the regex pattern.The regex pattern is complex and handles various edge cases. Consider adding comments to explain:
- The purpose of ANSI color code handling (
(?:\\x1b\\[\\d+m)?)- Example matches to demonstrate the pattern
Add documentation above the pattern:
{ "problemMatcher": [ { "owner": "actionlint", "pattern": [ { + "// Matches actionlint output format with ANSI color codes", + "// Example match: 'workflow.yml:10:5: Error in workflow [E001]'", "regexp": "^(?:\\x1b\\[\\d+m)?(.+?)(?:\\x1b\\[\\d+m)*:(?:\\x1b\\[\\d+m)*(\\d+)(?:\\x1b\\[\\d+m)*:(?:\\x1b\\[\\d+m)*(\\d+)(?:\\x1b\\[\\d+m)*: (?:\\x1b\\[\\d+m)*(.+?)(?:\\x1b\\[\\d+m)* \\[(.+?)\\]$",.github/workflows/main.yml (1)
70-75: Consider refactoring duplicated bundle installation logic.The bundle installation logic is duplicated in three places. Consider creating a reusable composite action to reduce duplication.
Example composite action:
# .github/actions/bundle-install/action.yml name: 'Bundle Install' description: 'Install Ruby gems with caching' inputs: working-directory: description: 'Directory to run bundle install in' required: false default: '.' runs: using: "composite" steps: - shell: bash working-directory: ${{ inputs.working-directory }} run: | bundle lock --add-platform 'x86_64-linux' if ! bundle check --path=vendor/bundle; then bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 fiUsage:
- - name: Install Ruby Gems for dummy app - run: | - cd spec/dummy - bundle lock --add-platform 'x86_64-linux' - if ! bundle check --path=vendor/bundle; then - bundle _2.5.9_ install --path=vendor/bundle --jobs=4 --retry=3 - fi + - name: Install Ruby Gems for dummy app + uses: ./.github/actions/bundle-install + with: + working-directory: spec/dummyAlso applies to: 158-162, 164-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.circleci/config.yml(0 hunks).github/actionlint-matcher.json(1 hunks).github/workflows/examples.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(2 hunks).github/workflows/main.yml(4 hunks).travis.yml(0 hunks)
💤 Files with no reviewable changes (2)
- .circleci/config.yml
- .travis.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/lint-js-and-ruby.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/examples.yml
64-64: shellcheck reported issue in this script: SC2086:info:1:41: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/main.yml
81-81: shellcheck reported issue in this script: SC2086:info:1:41: Double quote to prevent globbing and word splitting
(shellcheck)
141-141: shellcheck reported issue in this script: SC2086:info:1:41: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (3)
.github/actionlint-matcher.json (1)
1-17: LGTM! The problem matcher configuration is well-structured.The implementation correctly handles ANSI color codes and captures all necessary components for error reporting.
.github/workflows/examples.yml (1)
64-64: LGTM! Well-structured workflow improvements.The changes improve the workflow by:
- Using the modern
$GITHUB_OUTPUTsyntax for setting outputs- Adding conditional bundle installation to optimize CI performance
Also applies to: 72-76
🧰 Tools
🪛 actionlint (1.7.4)
64-64: shellcheck reported issue in this script: SC2086:info:1:41: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/main.yml (1)
77-77: LGTM! Workflow syntax and logic improvements.The changes correctly:
- Use modern
$GITHUB_OUTPUTsyntax- Use proper numeric comparison with
-lt- Use consistent quoting for environment variables
Also applies to: 79-79, 81-81, 141-141, 175-175, 185-185
| @@ -61,15 +61,19 @@ jobs: | |||
| path: vendor/bundle | |||
| key: package-app-gem-cache-${{ hashFiles('react_on_rails.gemspec') }}-${{ hashFiles('Gemfile.development_dependencies') }}-${{ matrix.versions }} | |||
| - id: get-sha | |||
| run: echo "::set-output name=sha::$(git rev-parse HEAD)" | |||
| run: echo "sha=\"$(git rev-parse HEAD)\"" >> $GITHUB_OUTPUT | |||
| - name: Install Node modules with Yarn for renderer package | |||
| run: | | |||
| yarn install --no-progress --no-emoji | |||
| sudo yarn global add yalc | |||
| - name: yalc publish for react-on-rails | |||
| run: yalc publish | |||
| - name: Install Ruby Gems for package | |||
There was a problem hiding this comment.
I don't like this duplication across workflow files, but I checked https://docs.github.com/en/actions/sharing-automations/avoiding-duplication and the setup there seems even worse.
alexeyr-ci
force-pushed
the
alexeyr/ci-workflow-fix
branch
from
5e5c6e2 to
a2ed178
Compare
alexeyr-ci
force-pushed
the
alexeyr/ci-workflow-fix
branch
from
a2ed178 to
52a9e82
Compare
| if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q '^.github/workflows'; then | ||
| echo "::add-matcher::.github/actionlint-matcher.json" | ||
| bash <(curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash) | ||
| SHELLCHECK_OPTS="-S warning" ./actionlint -color |
There was a problem hiding this comment.
Should info messages be included as well? I fixed them in this PR, and GH Actions problem matchers only support warning and error as severities.
This looks like a good idea, though |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.circleci/config.yml(0 hunks).github/actionlint-matcher.json(1 hunks).github/workflows/examples.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(2 hunks).github/workflows/main.yml(4 hunks).travis.yml(0 hunks)
💤 Files with no reviewable changes (2)
- .circleci/config.yml
- .travis.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/examples.yml
- .github/workflows/main.yml
- .github/actionlint-matcher.json
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
🔇 Additional comments (4)
.github/workflows/lint-js-and-ruby.yml (4)
55-60: LGTM! Improved bundle install efficiency.The refactored installation script with conditional checking and platform locking is a good improvement for efficiency and consistency.
62-62: LGTM! Proper environment variable quoting.Good practice to quote the RAILS_ENV value to prevent potential shell interpretation issues.
97-98: LGTM! Good error reporting setup.The problem matcher configuration and colored output will improve the visibility of linting issues in the GitHub Actions UI.
92-93: Consider security implications of curl | bash pattern.The current pattern of downloading and executing a script directly from the internet poses security risks. Consider:
- Downloading the script first
- Verifying its checksum
- Then executing it
✅ Verification successful
The current implementation is secure enough for CI/CD usage
While the curl|bash pattern can be risky, this specific implementation is secure because it:
- Downloads from GitHub's release infrastructure using HTTPS
- Uses immutable, signed release artifacts
- Is widely adopted (2.9k+ stars) and actively maintained
The script includes proper error handling and validation, making it suitable for CI/CD environments. If additional security is desired, you could pin to a specific version instead of using the latest.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the download script has changed recently # This helps verify if we need to update our security review curl -sI https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash | grep -i 'last-modified'Length of output: 123
Script:
#!/bin/bash # Fetch and examine the download script echo "=== Download Script Content ===" curl -s https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash echo -e "\n=== GitHub Repository Stats ===" gh repo view rhysd/actionlint --json stargazerCount,updatedAt echo -e "\n=== Usage in Popular Repositories ===" # Search for repositories using this script in their workflows gh search code "curl https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash" --limit 5Length of output: 5374
| git fetch origin ${{ github.event.pull_request.base.sha }} | ||
| if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q '^.github/workflows'; then | ||
| echo "changed=true" >> "$GITHUB_OUTPUT" | ||
| actionlint_version=$(curl -s https://api.github.com/repos/rhysd/actionlint/releases/latest | jq -r .tag_name) | ||
| echo "actionlint_version=\"$actionlint_version\"" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
Add error handling for push events.
The git fetch command will fail for push events as github.event.pull_request.base.sha is only available in pull_request events. Consider adding a condition:
-git fetch origin ${{ github.event.pull_request.base.sha }}
-if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q '^.github/workflows'; then
+if [[ "${{ github.event_name }}" == "pull_request" ]]; then
+ git fetch origin ${{ github.event.pull_request.base.sha }}
+ base_sha="${{ github.event.pull_request.base.sha }}"
+else
+ git fetch origin ${{ github.event.before }}
+ base_sha="${{ github.event.before }}"
+fi
+if git diff --name-only "$base_sha" ${{ github.sha }} | grep -q '^.github/workflows'; then📝 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.
| git fetch origin ${{ github.event.pull_request.base.sha }} | |
| if git diff --name-only ${{ github.event.pull_request.base.sha }} ${{ github.sha }} | grep -q '^.github/workflows'; then | |
| echo "changed=true" >> "$GITHUB_OUTPUT" | |
| actionlint_version=$(curl -s https://api.github.com/repos/rhysd/actionlint/releases/latest | jq -r .tag_name) | |
| echo "actionlint_version=\"$actionlint_version\"" >> "$GITHUB_OUTPUT" | |
| fi | |
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | |
| git fetch origin ${{ github.event.pull_request.base.sha }} | |
| base_sha="${{ github.event.pull_request.base.sha }}" | |
| else | |
| git fetch origin ${{ github.event.before }} | |
| base_sha="${{ github.event.before }}" | |
| fi | |
| if git diff --name-only "$base_sha" ${{ github.sha }} | grep -q '^.github/workflows'; then | |
| echo "changed=true" >> "$GITHUB_OUTPUT" | |
| actionlint_version=$(curl -s https://api.github.com/repos/rhysd/actionlint/releases/latest | jq -r .tag_name) | |
| echo "actionlint_version=\"$actionlint_version\"" >> "$GITHUB_OUTPUT" | |
| fi |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Fixes:
Also adds Actionlint to CI to prefer future problems.
Pull Request checklist
Add/update test to cover these changesUpdate documentationUpdate CHANGELOG fileOther Information
A rare case where CodeRabbit gave useful suggestions on a previous PR (thanks to running actionlint itself 😄)
This change is
Summary by CodeRabbit