-
Notifications
You must be signed in to change notification settings - Fork 16
refactor setup/action.yml to use setup-compact-action #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors GitHub Actions workflows to externalize Compact compiler setup. It removes the gh-token input parameter and manual compiler installation logic from the setup action, replaces them with a dependency on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/actions/setup/action.yml (2)
46-46: Hardcoded version should be parameterized for maintainability.The compact-version
"0.26.0"is hardcoded in the action. To improve maintainability and reduce friction for version updates:
- Consider accepting
compact-versionas an input parameter to the setup action with a sensible default- Or centralize version management in a dedicated file (e.g.,
.github/versions.jsonor similar)- This allows workflows to override the version without editing the action definition
Example parameterization:
inputs: skip-compact: description: "Skip Compact compiler installation" required: false default: "false" + compact-version: + description: "Version of Compact compiler to install" + required: false + default: "0.26.0" runs: using: "composite" steps: - name: Setup Compact Compiler if: ${{ inputs.skip-compact != 'true' }} uses: midnightntwrk/setup-compact-action@4130145456ad3f45934788dd4a65647eb283e658 with: - compact-version: "0.26.0" + compact-version: ${{ inputs.compact-version }}
1-2: Update action description to reflect new external dependency.The description "Sets up the environment with yarn, Node.js, turbo, and Compact compiler" is accurate but could be more specific about the new external action dependency. Consider updating to:
description: "Sets up the environment with yarn, Node.js, turbo, and Compact compiler via setup-compact-action"This makes the dependency explicit for users of the composite action.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/actions/setup/action.yml(1 hunks).github/workflows/checks.yml(1 hunks).github/workflows/codeql.yml(0 hunks).github/workflows/prepare-release.yml(1 hunks).github/workflows/release.yml(0 hunks).github/workflows/test.yml(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/release.yml
- .github/workflows/codeql.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Test Suite
🔇 Additional comments (4)
.github/workflows/prepare-release.yml (1)
16-26: Action version updates are appropriate.The pinned action versions (checkout@v6.0.1, setup-node@v6.1.0) are routine maintenance updates with SHAs correctly pinned for reproducibility.
.github/workflows/test.yml (1)
40-41: Verify the removal of "with retry" semantics.The step was renamed from "Compile contracts (with retry)" to "Compile contracts" and the
with:block was removed. Please confirm whether this change intentionally removes explicit retry logic or ifturbo compacthas built-in resilience. If retries were important for reliability, consider whetherturboprovides fallback mechanisms or if external retry wrapping is still needed..github/workflows/checks.yml (1)
23-26: Setup simplification is appropriate.Removal of COMPACT_INSTALLER_URL and gh-token aligns with the external action approach. The quote style change is neutral.
.github/actions/setup/action.yml (1)
42-46: Assess the impact of removed validation and error-handling steps.The previous manual setup likely included post-install validation checks (compiler/language version verification) and explicit error handling. The new external action may not provide equivalent guarantees. Confirm that:
- The external action validates successful compiler installation
- Setup failures are properly surfaced in workflow logs
- There are no silent failures that could lead to incorrect compilation
Consider adding a verification step after the setup if the external action doesn't provide built-in validation.
| - name: Compile contracts | ||
| run: turbo compact --filter=@openzeppelin/compact-contracts | ||
|
|
||
| - name: Run type checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For changed tests compared to HEAD..main i believe you can do turbo test --affected or --filter options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for letting me know about that feature. I'll take a look and see if that's behavior we want
Refactors our setup/action.yml to use the setup-compact-action which does the same thing as our manual configuration while greatly simplifying the action.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.