chore: migrate deployment types to shared-schemas (#17)#29
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR bumps Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
src/commands/deployments/deploy.ts
Outdated
| deployment = (await statusRes.json()) as DeploymentSchema; | ||
|
|
||
| if (deployment.status === 'ready' || deployment.status === 'READY') { | ||
| if (deployment.status === 'READY') { |
There was a problem hiding this comment.
[P1] Keep polling compatible with lowercase deployment statuses — src/commands/deployments/deploy.ts:130-146
If the OSS API still returns the lowercase statuses that the previous code explicitly handled (ready, error, canceled), this loop no longer recognizes successful or terminal deployments. In that case insforge deployments deploy will spin until the 2-minute timeout even though the deployment has already finished, and failed deployments will be reported as timeouts instead of surfacing the real error.
Suggestion: convert status to UpperCase then compare.
|
Hey @jwfing, Status is now normalized with .toUpperCase() before comparison in both the polling loop and the final isReady check. |
Closes #17
What
Migrates custom deployment type definitions in the CLI to use shared-schemas from
@insforge/shared-schemas, replacing local interfaces with canonical types.Why
Issue #17 asks to unify custom types to shared-schemas defined types. Local types were duplicating definitions already available in shared-schemas, and in the case of
DeploymentMetadata, the local type had stale field names causing the metadata command to silently display nulls.How
Types migrated (4):
CreateDeploymentResponse-- identical structure, drop-in replacementStartDeploymentRequest-- identical structure, drop-in replacementSiteDeployment→DeploymentSchema-- status enum normalized to uppercase, error extraction moved from top-levelerrorfield tometadata.error.errorMessage, removed staledeploymentUrlfieldDeploymentMetadata→DeploymentMetadataResponse-- fixes a bug where the CLI useddomain/slug/deploymentUrlbut the API returnsdefaultDomainUrl/customDomainUrlTypes kept local (2):
FunctionResponse/FunctionDeploymentResult-- these exist in shared-schemas source but the published npm package does not export them yet. Can be migrated once published.Other changes:
getDeploymentError()helper insrc/lib/errors.tsfor clean error extraction fromDeploymentSchema.metadata, reused indeploy.tsandstatus.ts@insforge/shared-schemasfrom^1.1.43to^1.1.44(required forDeploymentMetadataResponseexport)How to Test
insforge deployments metadata-- displaysDefault Domain URLandCustom Domain URLfieldsinsforge deployments status <id>-- displaysURL(single field, no moredeploymentUrlfallback) and extracts errors frommetadata.error.errorMessageSummary by CodeRabbit
Bug Fixes
Chores
Refactor