Update deployment scripts to use standardised env vars #1608
Conversation
WalkthroughThe changes primarily focus on improving the deployment process of the testnet on Azure and the recovery of network funds. This is achieved through updates to environment variables, Docker setup, Azure CLI commands, and VM creation. The changes also include the addition of a new test function for sending funds in a blockchain network. The modifications seem to be aimed at enhancing maintainability, consistency, and security. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (6)
- .github/workflows/manual-deploy-testnet-faucet.yml (3 hunks)
- .github/workflows/manual-deploy-testnet-l1.yml (3 hunks)
- .github/workflows/manual-deploy-testnet-l2.yml (10 hunks)
- .github/workflows/manual-recover-network-funds.yml (1 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (6 hunks)
- integration/networktest/tests/helpful/send_funds_test.go (1 hunks)
Files skipped from review due to trivial changes (4)
- .github/workflows/manual-deploy-testnet-faucet.yml
- .github/workflows/manual-deploy-testnet-l2.yml
- .github/workflows/manual-recover-network-funds.yml
- integration/networktest/tests/helpful/send_funds_test.go
Additional comments (Suppressed): 11
.github/workflows/manual-deploy-testnet-l1.yml (5)
- 37-50: The new hunk introduces a step to print GitHub variables for debugging and reference. This is a good practice as it helps in understanding the state of the environment variables at the time the job ran. However, there seems to be a duplicate line for
ACCOUNT_ADDR_WORKER. Please verify if this is intentional or a mistake.- echo "ACCOUNT_ADDR_WORKER=${{ vars.ACCOUNT_ADDR_WORKER }}" + echo "ACCOUNT_ADDR_L1_BRIDGE_TEST=${{ vars.ACCOUNT_ADDR_L1_BRIDGE_TEST }}"
71-72: The Docker build and push commands have been simplified. The new hunk only builds and pushes one Docker image, whereas the old hunk built and pushed two Docker images. Ensure that this change does not affect any dependencies that might be expecting the second Docker image.
79-86: The deletion of deployed VMs and their dependencies has been updated to use the new environment variable names. Ensure that these new environment variables are correctly set in the GitHub environment.
93-99: The VM creation step has been updated to use the new environment variable names. Ensure that these new environment variables are correctly set in the GitHub environment.
105:
The port opening step has been updated to use the new environment variable names. Ensure that these new environment variables are correctly set in the GitHub environment.
- 136-143: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [116-143]
The VM start command has been updated to use the new environment variable names. Ensure that these new environment variables are correctly set in the GitHub environment.
.github/workflows/manual-upgrade-testnet-l2.yml (6)
52-72: The environment variable names have been updated for better clarity and consistency. Ensure that these new names are reflected everywhere in the codebase where they are used.
77-83: The Azure resource tag used to fetch VM hostnames has been updated. Make sure that the new tag is correctly assigned to the VMs.
127-139: The lookup keys for node addresses and private keys have been updated. Ensure that these new keys are correctly set in the GitHub secrets.
156-162: The Azure hostname prefix used in the VM run command has been updated. Make sure that the new prefix is correctly used in the VM names.
171-184: The Docker image tags and environment variables used in the Docker run command have been updated. Ensure that these new tags and variables are correctly set in the Docker images and GitHub secrets.
196-200: The Azure resource name used in the health check script has been updated. Make sure that the new name is correctly used in the VM names.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (3)
- .github/workflows/manual-deploy-testnet-l2.yml (11 hunks)
- .github/workflows/manual-recover-network-funds.yml (1 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (8 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/manual-deploy-testnet-l2.yml
Additional comments (Suppressed): 8
.github/workflows/manual-recover-network-funds.yml (1)
- 52-52: The private key environment variable name has been changed from
WORKER_PKtoACCOUNT_PK_WORKER. Ensure that the new environment variable is set correctly in the GitHub secrets.- -private_key=${{ secrets.WORKER_PK }} \ + -private_key=${{ secrets.ACCOUNT_PK_WORKER }} \.github/workflows/manual-upgrade-testnet-l2.yml (7)
52-72: The environment variable names have been updated for better consistency and clarity. Ensure that these new names are reflected across all scripts and code that reference them.
77-83: The Azure resource tag used to fetch VM hostnames has been updated. Make sure the new tag is correctly applied to the relevant Azure resources.
90-96: No significant changes, just formatting adjustments.
101-109: The Docker image build tags have been updated. Ensure these new tags are used consistently across all Docker-related operations.
127-139: The lookup keys for node addresses and private keys have been updated. Make sure these new keys are correctly set in the GitHub secrets or wherever they are stored.
171-186: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [156-184]
The command to upgrade the Obscuro nodes has been updated with new environment variable names. Ensure these new names are used consistently across all scripts and code that reference them.
- 196-202: The hostnames used in the health check script have been updated. Make sure these new hostnames are correctly set in the Azure resources.
nice! |
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (5)
- .github/workflows/manual-deploy-testnet-faucet.yml (3 hunks)
- .github/workflows/manual-deploy-testnet-l1.yml (3 hunks)
- .github/workflows/manual-deploy-testnet-l2.yml (11 hunks)
- .github/workflows/manual-recover-network-funds.yml (2 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (8 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/manual-deploy-testnet-l2.yml
Additional comments (Suppressed): 13
.github/workflows/manual-deploy-testnet-faucet.yml (1)
- 48-53: The new hunk removes the steps that set environment variables for different testnets. If these environment variables are not set elsewhere in the workflow, this could cause issues when building and deploying the Docker image. Please verify that these environment variables are set correctly elsewhere in the workflow.
.github/workflows/manual-deploy-testnet-l1.yml (5)
- 34-49: The new hunk introduces a step to print GitHub variables for debugging and reference. This is a good practice for debugging purposes. However, it seems like the environment variable setting steps for different testnet types (uat-testnet and dev-testnet) have been removed. If these environment variables are set elsewhere in the code, then this change is fine. Otherwise, this could lead to undefined variables later in the code.
+ - name: 'Print GitHub variables' + # This is a useful record of what the environment variables were at the time the job ran, for debugging and reference + run: | + echo "GitHub Variables = ${{ toJSON(vars) }}"
- 60-64: The Docker build command has been updated to use a single tag instead of two. If the second tag is not used elsewhere, this change is fine. However, if the second tag is used elsewhere in the code, this could lead to issues.
- DOCKER_BUILDKIT=1 docker build -t ${{env.ETH2NETWORK_BUILD_TAG}} -t ${{env.L1_DOCKER_BUILD_TAG}} -f testnet/eth2network.Dockerfile . - docker push ${{env.L1_DOCKER_BUILD_TAG}} - docker push ${{env.ETH2NETWORK_BUILD_TAG}} + DOCKER_BUILDKIT=1 docker build -t ${{ vars.DOCKER_BUILD_TAG_ETH2NETWORK }} -f testnet/eth2network.Dockerfile . + docker push ${{ vars.DOCKER_BUILD_TAG_ETH2NETWORK }}
67-78: The steps to delete deployed VMs and their dependencies have been updated to use the new environment variable names. This is a good change for consistency and clarity.
81-92: The VM creation step has been updated to use the new environment variable names. This is a good change for consistency and clarity.
60-111: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [104-135]
The step to start the L1 network on the Azure VM has been updated to use the new environment variable names. This is a good change for consistency and clarity.
.github/workflows/manual-recover-network-funds.yml (3)
29-35: The addition of a step to print GitHub variables is a good practice for debugging and reference. However, the variable
varsis not defined in this context. Please ensure thatvarsis correctly defined and contains the necessary information.46-49: The Docker build and push commands have been updated to use the new variable name
L2_HARDHATDEPLOYER_DOCKER_BUILD_TAG. Ensure that this variable is correctly defined and initialized before its usage.55-60: The command-line arguments for the Go program have been updated to use the new variable names
L1_HTTP_URLandACCOUNT_PK_WORKER. Ensure that these secrets are correctly set in the GitHub secrets..github/workflows/manual-upgrade-testnet-l2.yml (4)
65-68: The variable
AZURE_RESOURCE_TAG_L2is used instead ofRESOURCE_TAG_NAME. Ensure that the new variable is correctly defined and holds the expected value.115-124: The lookup keys have been renamed. Make sure that the new keys
ACCOUNT_PK_NODE_0,ACCOUNT_PK_NODE_1,ACCOUNT_ADDR_NODE_0,ACCOUNT_ADDR_NODE_1, andL1_WS_URL_0are correctly defined and hold the expected values.156-171: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [144-169]
The variable
AZURE_RESOURCE_NAME_L2is used instead ofRESOURCE_TESTNET_NAMEandACCOUNT_ADDR_NODE_0is used instead ofNODE_WALLET_ADDR_0. Ensure that the new variables are correctly defined and hold the expected values.
- 184-185: The variable
AZURE_RESOURCE_NAME_L2is used instead ofRESOURCE_TESTNET_NAME. Ensure that the new variable is correctly defined and holds the expected value.
| jobs: | ||
| build-and-deploy: | ||
| runs-on: ubuntu-latest | ||
| environment: | ||
| name: ${{ github.event.inputs.testnet_type }} | ||
| steps: | ||
| - name: 'Print GitHub variables' | ||
| # This is a useful record of what the environment variables were at the time the job ran, for debugging and reference | ||
| run: | | ||
| echo "GitHub Variables = ${{ toJSON(vars) }}" | ||
|
|
||
| - run: echo "Workflow_dispatch inputs ${{ github.event.inputs.testnet_type }}" | ||
| - run: echo "Workflow_call inputs ${{ inputs.testnet_type }}" | ||
|
|
There was a problem hiding this comment.
The new hunk introduces an environment variable based on the testnet_type input and a step to print GitHub variables for debugging. However, the vars object used in line 41 is not defined anywhere. This will cause an error when the workflow runs. You should replace vars with github to print all GitHub context variables.
- echo "GitHub Variables = ${{ toJSON(vars) }}"
+ echo "GitHub Variables = ${{ toJSON(github) }}"|
|
||
| - name: Build and Push Docker Image | ||
| run: | | ||
| DOCKER_BUILDKIT=1 docker build -t ${{env.FAUCET_BUILD_TAG}} -f tools/faucet/Dockerfile . | ||
| docker push ${{env.FAUCET_BUILD_TAG}} | ||
| DOCKER_BUILDKIT=1 docker build -t ${{ vars.DOCKER_BUILD_TAG_FAUCET }} -f tools/faucet/Dockerfile . | ||
| docker push ${{ vars.DOCKER_BUILD_TAG_FAUCET }} | ||
|
|
||
| - name: 'Deploy to Azure Container Instances' | ||
| uses: 'azure/aci-deploy@v1' | ||
| with: | ||
| resource-group: ${{ secrets.RESOURCE_GROUP }} | ||
| dns-name-label: ${{ inputs.testnet_type }}-faucet | ||
| image: ${{ env.FAUCET_BUILD_TAG }} | ||
| image: ${{ vars.DOCKER_BUILD_TAG_FAUCET }} | ||
| name: ${{ inputs.testnet_type }}-faucet | ||
| location: 'uksouth' | ||
| restart-policy: 'Never' | ||
| environment-variables: PORT=80 | ||
| command-line: ./faucet --nodeHost ${{ env.TESTNET_ADDR }} --pk ${{ secrets.FAUCET_PK }} --jwtSecret ${{ secrets.FAUCET_JWT_SECRET }} --defaultAmount ${{ env.DEFAULT_FAUCET_AMOUNT }} | ||
| command-line: ./faucet --nodeHost ${{ vars.L2_RPC_URL_VALIDATOR }} --pk ${{ secrets.FAUCET_PK }} --jwtSecret ${{ secrets.FAUCET_JWT_SECRET }} --defaultAmount ${{ vars.FAUCET_PAY_AMOUNT }} | ||
| ports: '80' | ||
| cpu: 2 | ||
| memory: 2 |
There was a problem hiding this comment.
The new hunk changes the Docker build and push commands and the Azure Container Instances deployment command to use the new environment variable names. However, the vars object used in these commands is not defined anywhere. This will cause an error when the workflow runs. You should replace vars with the correct object or variable that contains these values.
- DOCKER_BUILDKIT=1 docker build -t ${{ vars.DOCKER_BUILD_TAG_FAUCET }} -f tools/faucet/Dockerfile .
- docker push ${{ vars.DOCKER_BUILD_TAG_FAUCET }}
+ DOCKER_BUILDKIT=1 docker build -t ${{ env.DOCKER_BUILD_TAG_FAUCET }} -f tools/faucet/Dockerfile .
+ docker push ${{ env.DOCKER_BUILD_TAG_FAUCET }}
- image: ${{ vars.DOCKER_BUILD_TAG_FAUCET }}
+ image: ${{ env.DOCKER_BUILD_TAG_FAUCET }}
- command-line: ./faucet --nodeHost ${{ vars.L2_RPC_URL_VALIDATOR }} --pk ${{ secrets.FAUCET_PK }} --jwtSecret ${{ secrets.FAUCET_JWT_SECRET }} --defaultAmount ${{ vars.FAUCET_PAY_AMOUNT }}
+ command-line: ./faucet --nodeHost ${{ env.L2_RPC_URL_VALIDATOR }} --pk ${{ secrets.FAUCET_PK }} --jwtSecret ${{ secrets.FAUCET_JWT_SECRET }} --defaultAmount ${{ env.FAUCET_PAY_AMOUNT }}| echo "BATCH_INTERVAL=${{vars.BATCH_INTERVAL}}" | ||
| echo "L2_MAX_BATCH_INTERVAL=${{vars.L2_MAX_BATCH_INTERVAL}}" | ||
| echo "ROLLUP_INTERVAL=${{vars.ROLLUP_INTERVAL}}" | ||
| echo "GitHub Variables = ${{ toJSON(vars) }}"¡ |
There was a problem hiding this comment.
There seems to be a typo at the end of the line. The exclamation mark ¡ is not needed and may cause a syntax error.
- echo "GitHub Variables = ${{ toJSON(vars) }}"¡
+ echo "GitHub Variables = ${{ toJSON(vars) }}"
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- .github/workflows/manual-deploy-testnet-l1.yml (4 hunks)
- .github/workflows/manual-deploy-testnet-l2.yml (11 hunks)
- .github/workflows/manual-upgrade-testnet-l2.yml (8 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/manual-deploy-testnet-l1.yml
- .github/workflows/manual-deploy-testnet-l2.yml
Additional comments (Suppressed): 4
.github/workflows/manual-upgrade-testnet-l2.yml (4)
65-68: The variable
AZURE_DEPLOY_GROUP_L2is used instead ofRESOURCE_TAG_NAME. Ensure that the new variable is correctly set and contains the expected value.115-124: The variable names for the node private keys and addresses have been changed. Ensure that these new variables are correctly set in the GitHub secrets.
156-171: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [144-169]
The variable
AZURE_RESOURCE_PREFIXis used instead ofRESOURCE_STARTING_NAME. Also, the variableACCOUNT_ADDR_NODE_0is used instead ofNODE_WALLET_ADDR_0. Ensure that these new variables are correctly set and contain the expected values.
- 184-185: The variable
github.event.inputs.testnet_typeis used instead ofvars.RESOURCE_TESTNET_NAME. Ensure that this new variable is correctly set and contains the expected value.
| echo "BATCH_INTERVAL=${{vars.BATCH_INTERVAL}}" | ||
| echo "L2_MAX_BATCH_INTERVAL=${{vars.L2_MAX_BATCH_INTERVAL}}" | ||
| echo "ROLLUP_INTERVAL=${{vars.ROLLUP_INTERVAL}}" | ||
| echo "GitHub Variables = ${{ toJSON(vars) }}" |
There was a problem hiding this comment.
The new hunk is printing all environment variables, which could potentially expose sensitive information in logs. Consider reverting to the old hunk where only specific variables were printed.
- echo "GitHub Variables = ${{ toJSON(vars) }}"
+ echo "L2_ENCLAVE_DOCKER_BUILD_TAG=${{vars.L2_ENCLAVE_DOCKER_BUILD_TAG}}"
+ echo "L2_HARDHATDEPLOYER_DOCKER_BUILD_TAG=${{vars.L2_HARDHATDEPLOYER_DOCKER_BUILD_TAG}}"
+ echo "L2_HOST_DOCKER_BUILD_TAG=${{vars.L2_HOST_DOCKER_BUILD_TAG}}"
+ echo "RESOURCE_STARTING_NAME=${{vars.RESOURCE_STARTING_NAME}}"
+ echo "RESOURCE_TAG_NAME=${{vars.RESOURCE_TAG_NAME}}"
+ echo "RESOURCE_TESTNET_NAME=${{vars.RESOURCE_TESTNET_NAME}}"
+ echo "L1_CHAIN_ID=${{vars.L1_CHAIN_ID}}"
+ echo "NODE_WALLET_ADDR_0=${{vars.NODE_WALLET_ADDR_0}}"
+ echo "NODE_WALLET_ADDR_1=${{vars.NODE_WALLET_ADDR_1}}"
+ echo "WORKER_ADDR=${{vars.WORKER_ADDR}}"
+ echo "BATCH_INTERVAL=${{vars.BATCH_INTERVAL}}"
+ echo "L2_MAX_BATCH_INTERVAL=${{vars.L2_MAX_BATCH_INTERVAL}}"
+ echo "ROLLUP_INTERVAL=${{vars.ROLLUP_INTERVAL}}"
| @@ -215,7 +199,7 @@ jobs: | |||
| uses: azure/CLI@v1 | |||
| with: | |||
| inlineScript: | | |||
| az vm run-command invoke -g Testnet -n "${{vars.RESOURCE_STARTING_NAME}}-${{ matrix.host_id }}-${{ GITHUB.RUN_NUMBER }}" \ | |||
| az vm run-command invoke -g Testnet -n "${{ vars.AZURE_RESOURCE_PREFIX }}-${{ matrix.host_id }}-${{ GITHUB.RUN_NUMBER }}" \ | |||
There was a problem hiding this comment.
nit: AZURE_RESOURCE_PREFIX -> AZURE_RESOURCE_PREFIX_L2
There was a problem hiding this comment.
Not sure I agree with that one, feels like ideally we'd want to use the same prefix chars for the whole environment for any situation that needs a prefix char.
There was a problem hiding this comment.
Ah.. you mean that if it's sepolia then everything is S ? Independent of what it is.
Yeah fair 👍
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- .github/workflows/manual-deploy-testnet-l1.yml (4 hunks)
Additional comments (Suppressed): 6
.github/workflows/manual-deploy-testnet-l1.yml (6)
10-12: The internal addresses for the uat-testnet and dev-testnet have been updated. Ensure that these changes are reflected in all parts of the codebase where these addresses are used.
37-40: A new step has been added to print GitHub variables for debugging and reference. This is a good practice for debugging purposes.
63-64: The Docker build and push commands have been updated to use the new variable names. Ensure that these variables are correctly set in the GitHub environment.
71-78: The Azure CLI commands for deleting deployed VMs and their dependencies have been updated to use the new variable names. Ensure that these variables are correctly set in the GitHub environment.
85-91: The Azure CLI command for creating a new VM has been updated to use the new variable names. Ensure that these variables are correctly set in the GitHub environment.
131-135: The Docker run command has been updated to use the new variable names. Ensure that these variables are correctly set in the GitHub environment.
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
had a problem deploying
to
dev-testnet
GitHub Actions
Failure
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
BedrockSquirrel
temporarily deployed
to
dev-testnet
GitHub Actions
Inactive
Why this change is needed
Want to move all scripts onto the same pattern and what to set up a clearer naming scheme that groups related variables together in the github UI.
What changes were made as part of this PR
Update the main deployment scripts to use standardised env variable names (and the standard spacing around variables that the GH docs use).
New Var names
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks