ci: move the remaining canister deploys off dfx - #693
Merged
Conversation
The docs deploy moved to icp-cli in #692 and the cli canister was left on dfx so only one production path changed at a time. Move it across too, and generalise the composite action rather than keeping a second copy of the identity import, ID link and retry logic. Both canisters get the same baseline check — the custom domain still resolves — and the cli deploy adds a version-specific one: /tags/latest must name the new release and both tarballs must actually unpack, which is what `mops self update` and the mops-test.yml matrix consume. The identity import is now idempotent. `icp identity import` has no --force and errors on an existing name, so the second canister through the action would have failed on the identity the first one imported.
Contributor
Cursor AI review👀 HUMAN REVIEW REQUESTED — significant intended changes detected
Significant Changes Requiring Human Review
VerdictDecision: REQUEST_HUMAN_REVIEW Generated for commit 02d9650 |
`npm run deploy-staging` / `deploy-ic` were the last dfx callers. icp-cli has no way to declare a canister ID in icp.yaml, so scripts/link-canister-ids.mjs points its ID store at canister_ids.json first; the release action uses the same helper, so CI takes a path a maintainer can exercise locally. `deploy` now also passes --no-create: without a linked ID, dfx would have read canister_ids.json but icp-cli would happily create a brand new canister. Both mainnet environments are declared explicitly in icp.yaml. That keeps `bench` off mainnet, which dfx.json did with a `remote` id, and stops a bare deploy-staging from reaching docs/blog/cli — canister_ids.json gives those the same id on both networks, so a staging deploy of them was a production deploy under another name. dfx.json's last reader was frontend/vite.config.ts, for a canisters/* alias map pointing into a .dfx/ directory that no longer exists and that nothing imported. With it gone the frontend selects its network with ICP_ENVIRONMENT instead of DFX_NETWORK, and bakes the resolved value rather than the raw env var so the network flag cannot disagree with the ids next to it. deploy:main is dropped — it was deploy-local with an extra prompt.
dfx is gone from every deploy path, so DEVELOPMENT.md gets the commands that replace it and the rollback sequence, and AGENTS.md's "dfx version is pinned in dfx.json" rule is replaced by how canister IDs are linked. setup-dfx stays in mops-test.yml and setup-mops.yml, contrary to what the titles of those jobs suggest: mops 1.x and 2.x run test/storage-actor.test.mo on the dfx *replica*, because mops.toml pins no [toolchain] pocket-ic and those versions fall back to dfx. Pinning one would not rescue 1.0.0, which speaks only the PocketIC 4.0.0 API. Both now pass dfx-version explicitly — setup-dfx's `auto` read the version out of dfx.json, so with that file deleted it would have silently started installing the latest dfx. In mops-test.yml the install is skipped for the current CLI, which must not need dfx and now fails rather than borrowing one.
Resolve canister_ids.json from the script rather than the cwd, and pass the canister name to it through the environment like the action's other steps.
Review of the deploy rewrite found three gaps, none of which could target the wrong canister but all of which fail quietly. `npm run deploy` had picked up `-y`, which icp documents as skipping the Candid interface compatibility check. The dfx script it replaced passed no such flag, so upgrading the production registry canister with a narrowed or removed public method would have gone through unchallenged — on the one path where a human is present and no workflow runs. Dropped there, kept for deploy-local and CI. The CLI serve check was the only network assertion in the release without a retry, and it sits between the canister upload and the artifacts commit. A boundary-node blip would have left the canister ahead of the repo, with the next release generating releases.json from a stale base and a re-run republishing to npm. It retries like everything around it. The frontend build still emitted a bundle when canister ids could not be read: the catch logged and left the map empty, the defines were never emitted, and a catch-all `process.env` turned every lookup into undefined. Green build, SPA that reaches nothing. Non-local now throws, and verify-build asserts an id is actually baked into dist/bundle. Also: import the release identity unconditionally rather than probing for one by name, since a probe cannot tell whether it holds this key; take the domain check off a pipeline that can SIGPIPE curl; surface the cause when listing canisters fails; and drop a stale dfx.json mention from mops.toml.
icp-cli splits its store by network type: managed networks (local) go to
.icp/cache/, connected ones (ic, staging) to .icp/data/. The migration guide is
explicit that .icp/data/ "should be committed to version control. Only
.icp/cache/ should be gitignored" — the mainnet mappings are project state, not
machine state.
This repo ignored .icp/ wholesale and rebuilt the mapping on every deploy from
canister_ids.json, which is the runtime reconstruction of a file that is meant
to be in git. Committing .icp/data/mappings/{ic,staging}.ids.json deletes the
script, a CI step, two npm-script prefixes and the class of failure where the
relink is what breaks.
Verified against production without linking anything: in a checkout whose only
source of IDs is the committed mapping, `icp canister status docs -e ic`
resolves to iwejr-6iaaa-aaaal-qcema-cai and reaches mainnet, failing only on
the anonymous caller not being allowed to read status.
canister_ids.json stays: it also records IDs for canisters icp.yaml does not
manage, and the frontend build reads it.
icp-cli keeps mainnet ids in .icp/data/mappings/, which is committed, so canister_ids.json was a second source of truth for the same values. The frontend build now reads the icp mappings for every network instead of its own two-branch split. Three ids it held — dao-frontend, dao-backend, play-backend — have no source in this repo and nothing deploys them; they are dropped rather than carried, recoverable from git history if they ever matter. Also fixes the frontend build assertion: it matched any principal-shaped string, and cli/api/network.ts hardcodes the ic and staging endpoint ids into the bundle, so it passed even on a build with no mappings at all. It now asserts the specific main id read from the mappings file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The build guard was still a tautology on ic and staging: cli/api/network.ts hardcodes both endpoint ids, and those are the same principals the mappings give for `main`, so asserting the id is present proved nothing there. It now also rejects any unreplaced CANISTER_ID_* define, which is what a missed vite key actually leaves behind. ICP_ENVIRONMENT loses its silent `local` default. `icp deploy assets -e ic` run by hand sets nothing, so the old default baked local replica ids into a mainnet bundle without a word. Verified icp-cli forwards the variable into build steps, so deploy-ic is unaffected; svelte-check and the dev server pass it explicitly. Also pins the pre-built asset canister wasm by sha256 — everything else in icp.yaml is pinned, and this entry now runs on the mainnet cli deploy — and adds the missing prerelease guard on the docs deploy, which the comment above it already claimed was stable-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Requiring the variable broke CI's own Build frontend step, which set nothing — it would have failed every run, ten minutes in, after the replica and deploy steps. The local mappings already exist by then, so the step passes `local` explicitly. Same for the two documented build commands in AGENTS.md and the frontend-testing skill. The unreplaced-define check now derives its keys the way vite.config.ts derives them instead of pattern-matching: a loose /CANISTER_ID/ also hits MOPS_REGISTRY_CANISTER_ID, which cli/api/network.ts reads at runtime on purpose and which has to survive into the bundle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reusing icp-cli's ICP_ENVIRONMENT looked tidy but the two resolve differently: `icp deploy -e ic` targets ic, while an exported ICP_ENVIRONMENT=local still reaches vite. Local dev commonly exports it, so `icp deploy assets -e ic` could bake local replica ids into a mainnet bundle — silently, since verify-build.mjs reads the same variable and would confirm the wrong answer. `npm run deploy` now derives MOPS_FRONTEND_NETWORK from the value it passes to `-e`, so they cannot disagree, and a raw `icp deploy` leaves it unset and fails. Reverts the prerelease guard on the docs deploy. The preview notes point 3.x users at docs.mops.one/next/, which only exists if docs deploy on preview tags; "everything from here to the docs deploy" meant up to, not including. Comment reworded to say so. Also: verify-build.mjs anchors its define-key match (MOPS_REGISTRY_CANISTER_ID contains REGISTRY_CANISTER_ID, so renaming a canister to `registry` would have false-positived) and drops its dependency on the minifier's quote style; `npm run replica` clears stale local mappings, which otherwise survived a replica reset and let a build bake a dead canister id. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`npm run deploy-local` sets no frontend network and deploys only `main`, so it never reaches vite — a developer following that hint got the same error again. Both messages now name the build command verbatim. Also runs `npm run check` in code-quality: the frontend check script now depends on an env-var default, and only the husky hook was exercising it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The identity import lived inside deploy-canister, so it ran once per canister — which is why it needed the delete-then-import dance: `icp identity import` has no --force, and the second canister through the action would have failed on the identity the first one left behind. Both deploys run in the same job on the same runner, so one import serves both. icp-cli install moves up with it, since the import needs the binary. Neither is gated on prerelease — the docs deploy runs on preview tags. MOPS_IDENTITY_PEM now crosses exactly one step rather than being passed into the action twice, and the action drops to the three things that are actually per-canister: npm ci, deploy, verify the domain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deploying now needs
icp, neverdfx.dfx.json,dfx.schema.jsonandcanister_ids.jsonare deleted;icp.yamlis the only deploy config left. Thecommands keep their names:
Canister IDs move into icp-cli's own store. It cannot declare an ID in the
manifest, but it splits its state:
.icp/cache/is machine-local, while.icp/data/mappings/<environment>.ids.jsonismeant to be committed,
which is what lets a fresh clone deploy without relinking. Both mainnet mappings
are in this PR. Deploys pass
--no-create, so a missing mapping fails loudlyinstead of creating a second production canister.
Three IDs
canister_ids.jsonheld —dao-frontend,dao-backend,play-backend— have no source in this repo and nothing deploys them. Droppedrather than carried; git history has them.
Follow-up to #692, which moved
the docs canister and deliberately left everything else on dfx.
Two footguns closed on the way
icp.yamlnow declares both mainnet environments explicitly rather thaninheriting "every canister".
icexcludesbench, whichdfx.jsonkept offmainnet with a
remoteid.stagingcovers onlymainandassets:docs,blogandclihave no staging canister of their own, so a baredeploy-stagingwas a production deploy under another name.network:on anenvironment defaults to
local, so an environment namedicwithout it woulddeploy nothing, quietly — hence the comment in the file.
The frontend selects its network with
MOPS_FRONTEND_NETWORKinstead ofDFX_NETWORK, and it is required — no silent default. Deliberately noticp-cli's own
ICP_ENVIRONMENT:-ewins for icp but an exportedICP_ENVIRONMENTstill reaches vite, so the two could disagree and bake localreplica ids into a mainnet bundle.
npm run deployderives it from the valueit passes to
-e, so they cannot; a rawicp deploy assetsleaves it unsetand fails instead of shipping. Previously a build with no network set took ids
from the local replica while baking
MOPS_NETWORK = "ic".The build assertion was decorative
frontend/verify-build.mjschecked that the bundle contained someprincipal-shaped string. But
cli/api/network.tshardcodes the ic and stagingendpoint ids and the frontend bundles that module, so a principal is always
present — the check passed on a build with no canister mappings at all, which
is precisely the bundle it exists to reject. Confirmed by deleting the mappings
and rebuilding: exit 0, no ids baked in. It now asserts the specific
mainidread from the mappings file, and fails without them.
The composite action is parameterised, not duplicated
deploy-docsbecomesdeploy-canister, taking the canister name, its customdomain, and the directory whose
npm ciits build step needs. A second actionwould mean a copy of the identity import and the retry loop, and both canisters'
baseline health check is the same assertion — the custom domain resolves only
while
.well-known/ic-domainsis served, and that file is only uploaded becausean
.ic-assets.jsonopts dot-directories in. What is specific to theclideploy stays in
release.yml:/tags/latestmust name the new version and bothtarballs must unpack, which is what
mops self updateand themops-test.ymlmatrix consume.
icp-cli and the identity are installed and imported once for the job, not once
per canister. Both deploys run on the same runner, so a per-canister import
would only give
icp identity import— which has no--force— an existingname to fail on, and it keeps
MOPS_IDENTITY_PEMto a single step instead ofpassing it across the action boundary twice.
What is unchanged
Everything in
cli/that supports dfx users —mops sourcesas a packtool,mops toolchain init,mops watch --deploy,mops init's packtool write — isuntouched. A separate PR removes those, and it has to land after this one: we
cannot tell users to drop dfx while our own release still needs it.
setup-dfxstays inmops-test.ymlandsetup-mops.yml, which was the onething I expected to delete. Those jobs need dfx as a replica, not a compiler:
test/storage-actor.test.moruns in replica mode,mops.tomlpins no[toolchain] pocket-ic, and mops 1.x/2.x fall back to the dfx replica. Pinningpocket-icwould not rescuesetup-mops.yml, which runs mops 1.0.0, and 1.0.0speaks only the PocketIC 4.0.0 API. Both jobs now pass
dfx-versionexplicitly,because
setup-dfx'sautomode reads it out ofdfx.json(
jq -er '.dfx // ""' dfx.json) and with that file deleted would have silentlyinstalled the latest dfx. In
mops-test.ymlthe install is skipped for thecurrent CLI, so a regression that made 3.x need dfx fails there instead of
passing on a borrowed binary.
Argument forwarding through the nested
npm runalso survives — a previous AIreview flagged it as broken and it is not. npm 10 appends positionals to the
already-named inner script rather than reading them as a script name:
Rollback
The
clicanister, from a checkout of the previous release commit:Same shape for
docs. The failure message in the action prints it too.Unproven until a real release
Neither canister deploy can be exercised against mainnet from a PR. The local
replica covered the rest, including a full
clideploy throughicp.yaml'sclientry — the first time it has actually run. Untested until the nextcli-v*tag:icp deploy --no-createupgrading the liveclianddocscanisters, andthe asset sync diffing against their existing contents rather than an empty one.
MOPS_IDENTITY_PEM, and the delete-then-import thatnow runs for the second canister.
cli.mops.one/docs.mops.one.npm run deploy-staging/deploy-ic, which no workflow runs.