Skip to content

chore: Support --end <front|back> in PR-to-staging deployment script#3650

Merged
aterga merged 1 commit intomainfrom
arshavir/deploy-pr-to-beta-iife
Mar 6, 2026
Merged

chore: Support --end <front|back> in PR-to-staging deployment script#3650
aterga merged 1 commit intomainfrom
arshavir/deploy-pr-to-beta-iife

Conversation

@aterga
Copy link
Contributor

@aterga aterga commented Mar 5, 2026

Motivation

This is needed due to the recent II canister split. For now, only Staging-A comes with a frontend staging environment, but we could easily add more as needed.

Changes

  • Generalize scripts/deploy-pr-to-beta to support the new option --end (mandatory)

Tests

  • Tested manually using ./scripts/deploy-pr-to-beta --end front --staging-a 3647

@aterga aterga marked this pull request as ready for review March 5, 2026 17:20
@aterga aterga requested review from Copilot and sea-snake March 5, 2026 17:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

echo ""
echo "Configure install arguments (press Enter to keep each current value):"

# Parse individual fields from the Candid text output.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can't we parse and merge this with didc instead?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +186 to +198
# We use grep + sed to extract the value portion after "field_name = ".
local current_backend_canister_id
current_backend_canister_id=$(echo "$raw_config" | grep 'backend_canister_id' | sed 's/.*= *//;s/ *;$//')
local current_backend_origin
current_backend_origin=$(echo "$raw_config" | grep 'backend_origin' | sed 's/.*= *//;s/ *;$//')
local current_related_origins
current_related_origins=$(echo "$raw_config" | sed -n '/related_origins/,/}/p' | tr '\n' ' ' | sed 's/.*= *//;s/ *;[[:space:]]*$//')
local current_fetch_root_key
current_fetch_root_key=$(echo "$raw_config" | grep 'fetch_root_key' | sed 's/.*= *//;s/ *;$//')
local current_analytics_config
current_analytics_config=$(echo "$raw_config" | grep 'analytics_config' | sed 's/.*= *//;s/ *;$//')
local current_dummy_auth
current_dummy_auth=$(echo "$raw_config" | grep 'dummy_auth' | sed 's/.*= *//;s/ *;$//')
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field extraction from raw_config is done via grep ... | sed 's/.*= *//', which strips everything up to the last = on the matched line. If the /.config Candid text is formatted with multiple fields on one line (e.g. record { a = ...; b = ...; }), this will parse the wrong value and can produce an incorrect install argument. Consider parsing by anchoring the regex on the specific field name (capture after backend_canister_id = etc.), or avoid text parsing entirely by decoding into a structured value (e.g. via didc) before prompting.

Suggested change
# We use grep + sed to extract the value portion after "field_name = ".
local current_backend_canister_id
current_backend_canister_id=$(echo "$raw_config" | grep 'backend_canister_id' | sed 's/.*= *//;s/ *;$//')
local current_backend_origin
current_backend_origin=$(echo "$raw_config" | grep 'backend_origin' | sed 's/.*= *//;s/ *;$//')
local current_related_origins
current_related_origins=$(echo "$raw_config" | sed -n '/related_origins/,/}/p' | tr '\n' ' ' | sed 's/.*= *//;s/ *;[[:space:]]*$//')
local current_fetch_root_key
current_fetch_root_key=$(echo "$raw_config" | grep 'fetch_root_key' | sed 's/.*= *//;s/ *;$//')
local current_analytics_config
current_analytics_config=$(echo "$raw_config" | grep 'analytics_config' | sed 's/.*= *//;s/ *;$//')
local current_dummy_auth
current_dummy_auth=$(echo "$raw_config" | grep 'dummy_auth' | sed 's/.*= *//;s/ *;$//')
# Extract the value portion after "field_name =" up to the terminating ";".
local current_backend_canister_id
current_backend_canister_id=$(echo "$raw_config" | grep 'backend_canister_id' | sed -n 's/.*backend_canister_id *= *\([^;]*\);.*/\1/p')
local current_backend_origin
current_backend_origin=$(echo "$raw_config" | grep 'backend_origin' | sed -n 's/.*backend_origin *= *\([^;]*\);.*/\1/p')
local current_related_origins
current_related_origins=$(echo "$raw_config" | tr '\n' ' ' | sed -n 's/.*related_origins *= *\([^;]*\);.*/\1/p')
local current_fetch_root_key
current_fetch_root_key=$(echo "$raw_config" | grep 'fetch_root_key' | sed -n 's/.*fetch_root_key *= *\([^;]*\);.*/\1/p')
local current_analytics_config
current_analytics_config=$(echo "$raw_config" | grep 'analytics_config' | sed -n 's/.*analytics_config *= *\([^;]*\);.*/\1/p')
local current_dummy_auth
current_dummy_auth=$(echo "$raw_config" | grep 'dummy_auth' | sed -n 's/.*dummy_auth *= *\([^;]*\);.*/\1/p')

Copilot uses AI. Check for mistakes.
Comment on lines +220 to +225
echo "Encoding argument with didc..."
local encoded
encoded=$(didc encode \
-d ./src/internet_identity_frontend/internet_identity_frontend.did \
-t '(InternetIdentityFrontendInit)' \
"$candid_arg")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script now relies on didc for frontend deployment, but there’s no preflight check. If didc isn’t installed, the script will fail with a generic "command not found" due to set -e. Add an explicit command -v didc check when --end front is requested and emit a clear actionable error message (and possibly a hint on how to install it).

Copilot uses AI. Check for mistakes.
install "$CANISTER_ID" \
--mode upgrade \
--wasm "$ROOT_DIR/$ARTIFACT_NAME" \
${INSTALL_ARGS[@]+"${INSTALL_ARGS[@]}"}
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INSTALL_ARGS is an array, but it’s expanded unquoted (${INSTALL_ARGS[@]+"${INSTALL_ARGS[@]}"}). Unquoted array expansion re-enables word-splitting/globbing and can corrupt arguments (especially --argument if it ever contains whitespace/newlines). Prefer passing the array as "${INSTALL_ARGS[@]}" behind an explicit length check (or always, since the array is initialized).

Suggested change
${INSTALL_ARGS[@]+"${INSTALL_ARGS[@]}"}
"${INSTALL_ARGS[@]}"

Copilot uses AI. Check for mistakes.
[.workflow_runs[]
| select(.pull_requests[]?.number == ($PR|tonumber))
| select(.path == (".github/workflows/" + $WF))
| select(.status == "completed" and .conclusion == "success")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By filtering to only successful completed runs (select(.status == "completed" and .conclusion == "success")), the later failure case now means “no successful run found” rather than “no run found”. Consider updating the associated error message accordingly so users aren’t misled when runs exist but haven’t succeeded yet.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@sea-snake sea-snake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aterga aterga added this pull request to the merge queue Mar 6, 2026
Merged via the queue into main with commit ed29198 Mar 6, 2026
71 checks passed
@aterga aterga deleted the arshavir/deploy-pr-to-beta-iife branch March 6, 2026 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants