ci: action for manual file generation #400
Conversation
lumtis
requested review from
andresaiello,
brewmaster012,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughA new GitHub Actions workflow file named Changes
Possibly related PRs
Suggested reviewers
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: 2
🧹 Outside diff range and nitpick comments (3)
.github/workflows/generate-manual.yaml (3)
15-31: LGTM: Job setup and initial steps are well-structured.The job setup and initial steps are well-defined. Using
ubuntu-latestensures a stable and up-to-date environment. The Node.js setup and Foundry installation are handled efficiently.Consider using an LTS (Long Term Support) version of Node.js instead of 21.1.0, which is a current release. LTS versions are more stable and receive longer support. For example:
- node-version: "21.1.0" + node-version: "20.10.0" # Latest LTS version as of December 2023
33-45: LGTM: Dependency installation and environment setup are comprehensive.The installation of system and project dependencies is well-handled. The specific version of abigen ensures consistency across environments.
Consider improving the abigen installation process for better maintainability:
- Use variables for version numbers to make updates easier:
- name: Install specific version of abigen env: ABIGEN_VERSION: "1.11.5" ABIGEN_CHECKSUM: "<checksum>" # Add the correct checksum here run: | wget "https://gethstore.blob.core.windows.net/builds/geth-alltools-linux-amd64-${ABIGEN_VERSION}-a38f4108.tar.gz" echo "${ABIGEN_CHECKSUM} geth-alltools-linux-amd64-${ABIGEN_VERSION}-a38f4108.tar.gz" | sha256sum -c tar -zxvf "geth-alltools-linux-amd64-${ABIGEN_VERSION}-a38f4108.tar.gz" sudo mv "geth-alltools-linux-amd64-${ABIGEN_VERSION}-a38f4108/abigen" /usr/local/bin/
- Consider using a more robust installation method, such as downloading from GitHub releases if available.
76-76: Add a new line at the end of the file.To comply with YAML best practices and resolve the linter warning, add a new line at the end of the file.
labels: ["CI"] +🧰 Tools
🪛 actionlint
76-76: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/generate-manual.yaml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/generate-manual.yaml
52-52: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
52-52: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
52-52: shellcheck reported issue in this script: SC1047:error:14:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
52-52: shellcheck reported issue in this script: SC1072:error:14:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
52-52: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
52-52: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
76-76: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
.github/workflows/generate-manual.yaml
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/generate-manual.yaml (1)
1-13: LGTM: Workflow name and trigger are well-defined.The workflow name clearly describes its purpose, and the
workflow_dispatchevent with a branch input aligns well with the PR objectives. This setup allows for manual triggering and flexibility in choosing the target branch.
| - name: Generate Go packages and types | ||
| run: | | ||
| yarn generate | ||
|
|
||
| - name: Check for changes | ||
| id: check_changes | ||
| run: | | ||
| if git diff --exit-code --ignore-space-change --ignore-all-space --ignore-cr-at-eol -- pkg types; then | ||
| echo "Generated Go files are up-to-date." | ||
| echo "::set-output name=changes::false" | ||
| else | ||
| echo "Generated files are not up-to-date. Creating a PR." | ||
| echo "::set-output name=changes::true" | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git checkout -b update-generated-files | ||
| git add pkg types | ||
| git commit -m "Update generated Go files" | ||
| git push origin update-generated-files | ||
|
|
There was a problem hiding this comment.
LGTM: File generation and change detection process is well-implemented.
The file generation and change detection process is logically sound and aligns with the PR objectives.
The set-output command used in the script is deprecated. Replace it with the new syntax:
- echo "::set-output name=changes::false"
+ echo "changes=false" >> $GITHUB_OUTPUT
- echo "::set-output name=changes::true"
+ echo "changes=true" >> $GITHUB_OUTPUTConsider adding error handling for the yarn generate command:
- name: Generate Go packages and types
run: |
if ! yarn generate; then
echo "Error: Failed to generate files"
exit 1
fi🧰 Tools
🪛 actionlint
52-52: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
52-52: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
52-52: shellcheck reported issue in this script: SC1047:error:14:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
52-52: shellcheck reported issue in this script: SC1072:error:14:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
52-52: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
52-52: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
| - name: Create Pull Request | ||
| if: steps.check_changes.outputs.changes == 'true' | ||
| uses: peter-evans/create-pull-request@v5 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| branch: update-generated-files | ||
| base: ${{ github.event.inputs.branch }} | ||
| title: "ci: generate files" | ||
| body: | | ||
| This PR updates the auto-generated files with `yarn generate`. Please review the changes. | ||
| labels: ["CI"] |
There was a problem hiding this comment.
LGTM: Pull request creation process is well-defined.
The pull request creation process is efficient, using a predefined action and creating a PR only when changes are detected.
The YAML syntax for the PR body is incorrect. Fix it by using the | character for multi-line strings:
body: |
- This PR updates the auto-generated files with `yarn generate`. Please review the changes.
+ This PR updates the auto-generated files with `yarn generate`. Please review the changes.Consider adding more context to the PR body:
body: |
This PR updates the auto-generated files with `yarn generate`.
Changes were detected in the following directories:
- pkg
- types
Please review the changes carefully to ensure they align with expected updates.🧰 Tools
🪛 actionlint
76-76: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
[error] 76-76: no new line character at the end of file
(new-line-at-end-of-file)
![semgrep-code-zeta-chain[bot]](https://avatars.githubusercontent.com/u/86979844?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| run: | | ||
| if git diff --exit-code --ignore-space-change --ignore-all-space --ignore-cr-at-eol -- pkg types; then | ||
| echo "Generated Go files are up-to-date." | ||
| echo "::set-output name=changes::false" | ||
| else | ||
| echo "Generated files are not up-to-date. Creating a PR." | ||
| echo "::set-output name=changes::true" | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git checkout -b ${{ github.event.inputs.custom_branch_name }} | ||
| git add pkg types | ||
| git commit -m "ci: generate files" | ||
| git push origin ${{ github.event.inputs.custom_branch_name }} | ||
|
|
Check failure
Code scanning / Semgrep PRO
[Semgrep Code] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'): 'run-shell-injection' High
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
.github/workflows/generate-manual.yaml (3)
15-17: Consider adding a comment to explain the working directory.The
working-directoryis set to./v2, which may not be immediately clear to all users. Consider adding a comment to explain why this specific directory is chosen as the working directory for the workflow.
19-48: LGTM: Environment setup is comprehensive.The job setup and environment preparation are well-defined and use specific versions for consistency.
Consider simplifying the
abigeninstallation if a package manager option is available. This could reduce the number of steps and potential points of failure.
70-80: Enhance the pull request body for better context.Consider expanding the pull request body to provide more context about the changes. Here's a suggested improvement:
body: | This PR updates the auto-generated files using `yarn generate`. ## Changes - Updated Go packages in the `pkg` directory - Updated type definitions in the `types` directory Please review these changes carefully to ensure they align with expected updates. If you encounter any issues or have questions, please comment below.This enhanced PR body provides more specific information about what was changed and encourages thorough review, which can help streamline the review process.
🧰 Tools
🪛 actionlint
80-80: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/generate-manual.yaml (1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/generate-manual.yaml
56-56: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
56-56: shellcheck reported issue in this script: SC1047:error:14:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1072:error:14:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
80-80: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
.github/workflows/generate-manual.yaml
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: Semgrep Pro
.github/workflows/generate-manual.yaml
[failure] 56-69: [Semgrep Code] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'): 'run-shell-injection'
Using variable interpolation${{...}}withgithubcontext data in arun:step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code.githubcontext data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable withenv:to store the data and use the environment variable in therun:script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".
🔇 Additional comments (2)
.github/workflows/generate-manual.yaml (2)
1-13: LGTM: Workflow name and trigger are well-defined.The workflow name accurately describes its purpose, and the
workflow_dispatchevent with customizable inputs aligns well with the PR objectives for manual file generation.
1-80: Overall assessment: Well-implemented workflow with room for minor improvements.This GitHub Actions workflow successfully implements a manual file generation process that aligns with the PR objectives. It includes appropriate checks, file generation, and automated PR creation. The suggested improvements focus on:
- Enhancing security by sanitizing user inputs
- Updating deprecated GitHub Actions commands
- Improving error handling
- Providing more context in the generated pull request
Implementing these suggestions will further improve the robustness and maintainability of this workflow.
🧰 Tools
🪛 actionlint
56-56: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
56-56: shellcheck reported issue in this script: SC1047:error:14:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1072:error:14:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
80-80: expected scalar node for string value but found sequence node with "!!seq" tag
(syntax-check)
🪛 yamllint
[error] 80-80: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Check: Semgrep Pro
[failure] 56-69: [Semgrep Code] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'): 'run-shell-injection'
Using variable interpolation${{...}}withgithubcontext data in arun:step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code.githubcontext data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable withenv:to store the data and use the environment variable in therun:script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".
| - name: Generate Go packages and types | ||
| run: | | ||
| yarn generate | ||
|
|
||
| - name: Check for changes | ||
| id: check_changes | ||
| run: | | ||
| if git diff --exit-code --ignore-space-change --ignore-all-space --ignore-cr-at-eol -- pkg types; then | ||
| echo "Generated Go files are up-to-date." | ||
| echo "::set-output name=changes::false" | ||
| else | ||
| echo "Generated files are not up-to-date. Creating a PR." | ||
| echo "::set-output name=changes::true" | ||
| git config user.name "github-actions[bot]" |
There was a problem hiding this comment.
Update deprecated command and add error handling.
- The
set-outputcommand is deprecated. Replace it with the new syntax:
- echo "::set-output name=changes::false"
+ echo "changes=false" >> $GITHUB_OUTPUT
- echo "::set-output name=changes::true"
+ echo "changes=true" >> $GITHUB_OUTPUT- Consider adding error handling for the
yarn generatecommand:
- name: Generate Go packages and types
run: |
if ! yarn generate; then
echo "Error: Failed to generate files"
exit 1
fiThese changes will improve the workflow's compatibility with future GitHub Actions updates and make it more robust against potential errors.
🧰 Tools
🪛 actionlint
56-56: shellcheck reported issue in this script: SC1046:error:1:1: Couldn't find 'fi' for this 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1073:error:1:1: Couldn't parse this if expression. Fix to allow more checks
(shellcheck)
56-56: shellcheck reported issue in this script: SC1047:error:14:1: Expected 'fi' matching previously mentioned 'if'
(shellcheck)
56-56: shellcheck reported issue in this script: SC1072:error:14:1: Expected 'fi'. Fix any mentioned problems and try again
(shellcheck)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
56-56: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git checkout -b ${{ github.event.inputs.custom_branch_name }} | ||
| git add pkg types | ||
| git commit -m "ci: generate files" | ||
| git push origin ${{ github.event.inputs.custom_branch_name }} | ||
|
|
There was a problem hiding this comment.
Potential security risk: Sanitize user input for branch name.
The custom_branch_name input is used directly in a shell command, which could potentially lead to command injection if not properly sanitized. To mitigate this risk, consider sanitizing the input or using it in a safer context.
Here's a suggested improvement:
- name: Prepare branch name
run: |
SAFE_BRANCH_NAME=$(echo "${{ github.event.inputs.custom_branch_name }}" | sed 's/[^a-zA-Z0-9_-]/-/g')
echo "SAFE_BRANCH_NAME=${SAFE_BRANCH_NAME}" >> $GITHUB_ENV
- name: Create and push branch
run: |
git checkout -b $SAFE_BRANCH_NAME
git add pkg types
git commit -m "ci: generate files"
git push origin $SAFE_BRANCH_NAMEThis change sanitizes the branch name by replacing any characters that are not alphanumeric, underscore, or hyphen with a hyphen, reducing the risk of command injection.
🧰 Tools
🪛 GitHub Check: Semgrep Pro
[failure] 56-69: [Semgrep Code] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection'): 'run-shell-injection'
Using variable interpolation${{...}}withgithubcontext data in arun:step could allow an attacker to inject their own code into the runner. This would allow them to steal secrets and code.githubcontext data can have arbitrary user input and should be treated as untrusted. Instead, use an intermediate environment variable withenv:to store the data and use the environment variable in therun:script. Be sure to use double-quotes the environment variable, like this: "$ENVVAR".
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: "21.1.0" | ||
| registry-url: "https://registry.npmjs.org" | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@v1 | ||
|
|
||
| - name: Install dependencies | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y jq unzip | ||
| yarn install | ||
| forge soldeer update | ||
|
|
||
| - name: Install specific version of aibgen | ||
| run: | | ||
| wget https://gethstore.blob.core.windows.net/builds/geth-alltools-linux-amd64-1.11.5-a38f4108.tar.gz | ||
| tar -zxvf geth-alltools-linux-amd64-1.11.5-a38f4108.tar.gz | ||
| sudo mv geth-alltools-linux-amd64-1.11.5-a38f4108/abigen /usr/local/bin/ |
There was a problem hiding this comment.
We should install all these tools in a docker container then use that that container locally and in CI for a consistent experience
| git commit -m "ci: generate files" | ||
| git push origin ${{ github.event.inputs.custom_branch_name }} | ||
|
|
||
| - name: Create Pull Request |
There was a problem hiding this comment.
why not just push directly to the users branch?
Given
yarn generateoutput depends on machine setup, this action allow to easily create a PR to make the file generation depending on the setup in the CI.Creating it as I'm running into an issue where I can't find reason for different input (same Forge, solc, and abigen version as CI but the generated contract binary is slightly different)
Summary by CodeRabbit