-
Notifications
You must be signed in to change notification settings - Fork 38
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
Testnet Deployments: set enclave TESTMODE=true #1701
Conversation
WalkthroughThe changes involve the addition of a build argument Changes
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 X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/manual-deploy-testnet-l2.yml (1 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/manual-upgrade-testnet-l2.yml
Additional comments: 3
.github/workflows/manual-deploy-testnet-l2.yml (3)
70-70: The addition of
--build-arg TESTMODE=true
to thedocker build
command for the enclave image aligns with the PR's objective to resolve issues with testnet deployments. This change should enable test mode during the build, which is necessary for successful testnet deployments and upgrades.67-73: It's important to verify that the addition of the
TESTMODE
build argument does not interfere with any other steps or dependencies in the build process. This includes checking for any scripts or configurations that might behave differently when the enclave is in test mode.Verification successful
Based on the references found in the codebase, the addition of
TESTMODE=true
in the GitHub workflow files appears to be consistent with the existing usage of theTESTMODE
variable in Dockerfiles, testnet configurations, and Go source code. The Docker build process is designed to handle theTESTMODE
argument, and the change aligns with the PR's objective to enable test mode for testnet deployments.* 67-73: Ensure that the procedural checklist item for pre-merging PR checks is completed by the developer as outlined in the project's documentation. This is a non-code related but essential step in the PR process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any references to TESTMODE in scripts or configurations that might behave differently when the enclave is in test mode. rg --files-with-matches 'TESTMODE' | xargs rg 'TESTMODE'Length of output: 774
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.
lgtm
Why this change is needed
Testnet deployments and upgrades are currently failing because the enclave docker image is set to TESTMODE=false (the default).
This means it expects to find required flags from enclave.json but they're not there so it panics.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks