-
Notifications
You must be signed in to change notification settings - Fork 45
fix(deploy): add harness teardown to TUI deploy flow #1394
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { getHarness } from '../src/cli/aws/agentcore-harness.js'; | ||
| import { hasAwsCredentials, parseJsonOutput, prereqs, retry, spawnAndCollect } from '../src/test-utils/index.js'; | ||
| import { | ||
| cleanupStaleCredentialProviders, | ||
|
|
@@ -7,7 +8,7 @@ import { | |
| writeAwsTargets, | ||
| } from './e2e-helper.js'; | ||
| import { randomUUID } from 'node:crypto'; | ||
| import { mkdir, rm } from 'node:fs/promises'; | ||
| import { mkdir, readFile, rm } from 'node:fs/promises'; | ||
| import { tmpdir } from 'node:os'; | ||
| import { join } from 'node:path'; | ||
| import { afterAll, beforeAll, describe, expect, it } from 'vitest'; | ||
|
|
@@ -30,10 +31,11 @@ export function createHarnessE2ESuite(cfg: HarnessE2EConfig) { | |
| const providerLabel = | ||
| cfg.modelProvider === 'open_ai' ? 'OpenAI' : cfg.modelProvider === 'gemini' ? 'Gemini' : 'Bedrock'; | ||
|
|
||
| describe.sequential(`e2e: harness/${providerLabel} — create → deploy → invoke`, () => { | ||
| describe.sequential(`e2e: harness/${providerLabel} — create → deploy → invoke → teardown`, () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a check in these tests that remove all and deploy actually deletes the harness |
||
| let testDir: string; | ||
| let projectPath: string; | ||
| let harnessName: string; | ||
| let harnessId: string; | ||
|
|
||
| beforeAll(async () => { | ||
| if (!canRun) return; | ||
|
|
@@ -76,7 +78,8 @@ export function createHarnessE2ESuite(cfg: HarnessE2EConfig) { | |
|
|
||
| afterAll(async () => { | ||
| if (projectPath && hasAws) { | ||
| await teardownE2EProject(projectPath, harnessName, cfg.modelProvider); | ||
| // Teardown is tested as a step; this is a safety net in case earlier steps fail | ||
| await teardownE2EProject(projectPath, harnessName, cfg.modelProvider).catch((_: unknown) => undefined); | ||
| } | ||
| if (testDir) await rm(testDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 1000 }); | ||
| }, 600000); | ||
|
|
@@ -158,6 +161,62 @@ export function createHarnessE2ESuite(cfg: HarnessE2EConfig) { | |
| expect(harness, `Harness "${harnessName}" should appear in status`).toBeDefined(); | ||
| expect(harness!.deploymentState).toBe('deployed'); | ||
| expect(harness!.identifier, 'Deployed harness should have a harnessArn').toBeTruthy(); | ||
|
|
||
| // Capture harnessId for teardown verification | ||
| const statePath = join(projectPath, 'agentcore', '.cli', 'deployed-state.json'); | ||
| const stateJson = JSON.parse(await readFile(statePath, 'utf-8')) as { | ||
| targets?: { default?: { resources?: { harnesses?: Record<string, { harnessId: string }> } } }; | ||
| }; | ||
| const harnessEntry = stateJson.targets?.default?.resources?.harnesses?.[harnessName]; | ||
| if (harnessEntry) { | ||
| harnessId = harnessEntry.harnessId; | ||
| } | ||
| }, | ||
| 120000 | ||
| ); | ||
|
|
||
| it.skipIf(!canRun)( | ||
| 'remove all and deploy tears down harness', | ||
| async () => { | ||
| const removeResult = await runAgentCoreCLI(['remove', 'all', '--yes', '--json'], projectPath); | ||
| expect(removeResult.exitCode, `Remove all failed: ${removeResult.stderr}`).toBe(0); | ||
|
|
||
| const removeJson = parseJsonOutput(removeResult.stdout) as { success: boolean }; | ||
| expect(removeJson.success).toBe(true); | ||
|
|
||
| const deployResult = await runAgentCoreCLI(['deploy', '--yes', '--json'], projectPath); | ||
| expect(deployResult.exitCode, `Teardown deploy failed: ${deployResult.stderr}`).toBe(0); | ||
|
|
||
| const deployJson = parseJsonOutput(deployResult.stdout) as { success: boolean }; | ||
| expect(deployJson.success).toBe(true); | ||
| }, | ||
| 600000 | ||
| ); | ||
|
|
||
| it.skipIf(!canRun)( | ||
| 'verifies harness is deleted from AWS', | ||
| async () => { | ||
| expect(harnessId, 'harnessId should have been captured').toBeTruthy(); | ||
|
|
||
| const region = process.env.AWS_REGION ?? 'us-east-1'; | ||
| await retry( | ||
| async () => { | ||
| try { | ||
| const result = await getHarness({ region, harnessId }); | ||
| expect(['DELETING', 'DELETED'], `Expected DELETING or DELETED, got ${result.harness.status}`).toContain( | ||
| result.harness.status | ||
| ); | ||
| } catch (err: unknown) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| expect( | ||
| message.includes('not found') || message.includes('ResourceNotFoundException'), | ||
| `Expected ResourceNotFound, got: ${message}` | ||
| ).toBe(true); | ||
| } | ||
| }, | ||
| 5, | ||
| 10000 | ||
| ); | ||
| }, | ||
| 120000 | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { ConfigIO } from '../../../../lib'; | ||
| import type { DeployedState, HarnessDeployedState } from '../../../../schema'; | ||
| import type { CdkToolkitWrapper, DeployMessage, SwitchableIoHost } from '../../../cdk/toolkit-lib'; | ||
| import { | ||
| buildDeployedState, | ||
|
|
@@ -14,9 +15,11 @@ import { | |
| } from '../../../cloudformation'; | ||
| import { DEFAULT_DEPLOY_ATTRS, computeDeployAttrs } from '../../../commands/deploy/utils.js'; | ||
| import { getErrorMessage, isChangesetInProgressError, isExpiredTokenError } from '../../../errors'; | ||
| import { isPreviewEnabled } from '../../../feature-flags'; | ||
| import { ExecLogger } from '../../../logging'; | ||
| import { performStackTeardown, setupTransactionSearch } from '../../../operations/deploy'; | ||
| import { getGatewayTargetStatuses } from '../../../operations/deploy/gateway-status'; | ||
| import { createDeploymentManager } from '../../../operations/deploy/imperative'; | ||
| import { deleteOrphanedABTests, setupABTests } from '../../../operations/deploy/post-deploy-ab-tests'; | ||
| import { | ||
| resolveConfigBundleComponentKeys, | ||
|
|
@@ -319,6 +322,38 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState | |
| setStackOutputs(outputs); | ||
|
|
||
| const existingState = await configIO.readDeployedState().catch(() => undefined); | ||
|
|
||
| // Post-CDK: deploy imperative resources (harness) — preview mode only | ||
| let deployedHarnesses: Record<string, HarnessDeployedState> | undefined; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this let is outside the feature flag it becomes dead code in the GA release. I think this specific pattern isn't very dangerous, but I'm worried this pattern may make it harder to cleanup since the codepaths are no longer isolated. |
||
| if (isPreviewEnabled()) { | ||
| const imperativeManager = createDeploymentManager(); | ||
| const imperativeDeployedState: DeployedState = existingState ?? { targets: {} }; | ||
| const imperativeContext = { | ||
| projectSpec: ctx.projectSpec, | ||
| target, | ||
| configIO, | ||
| deployedState: imperativeDeployedState, | ||
| cdkOutputs: outputs, | ||
| onProgress: (step: string, status: 'start' | 'done' | 'error') => { | ||
| logger.log(`${step}: ${status}`); | ||
| }, | ||
| }; | ||
|
|
||
| if (imperativeManager.hasDeployersForPhase('post-cdk', imperativeContext)) { | ||
| logger.startStep('Deploy harnesses'); | ||
| const postCdkResult = await imperativeManager.runPhase('post-cdk', imperativeContext); | ||
| const harnessResult = postCdkResult.results.get('harness'); | ||
| if (harnessResult?.state) { | ||
| deployedHarnesses = harnessResult.state as Record<string, HarnessDeployedState>; | ||
| } | ||
| if (!postCdkResult.success) { | ||
| logger.endStep('error', postCdkResult.error); | ||
| throw new Error(`Harness deployment failed: ${postCdkResult.error}`); | ||
| } | ||
| logger.endStep('success'); | ||
| } | ||
| } | ||
|
|
||
| let deployedState = buildDeployedState({ | ||
| targetName: target.name, | ||
| stackName: currentStackName, | ||
|
|
@@ -333,6 +368,7 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState | |
| policyEngines, | ||
| policies, | ||
| datasets, | ||
| harnesses: deployedHarnesses, | ||
| }); | ||
| await configIO.writeDeployedState(deployedState); | ||
|
|
||
|
|
@@ -673,6 +709,37 @@ export function useDeployFlow(options: DeployFlowOptions = {}): DeployFlowState | |
| await cdkToolkitWrapper.deploy(); | ||
|
|
||
| if (context?.isTeardownDeploy) { | ||
| // Teardown imperative resources (harnesses) before destroying the stack | ||
| if (isPreviewEnabled()) { | ||
| const teardownTarget = context.awsTargets[0]; | ||
| if (teardownTarget) { | ||
| const imperativeManager = createDeploymentManager(); | ||
| const teardownConfigIO = new ConfigIO(); | ||
| const existingTeardownState = await teardownConfigIO | ||
| .readDeployedState() | ||
| .catch(() => ({ targets: {} }) as DeployedState); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why do we need to cast the type here? is there something we're assuming? |
||
| const teardownContext = { | ||
| projectSpec: context.projectSpec, | ||
| target: teardownTarget, | ||
| configIO: teardownConfigIO, | ||
| deployedState: existingTeardownState, | ||
| onProgress: (step: string, status: 'start' | 'done' | 'error') => { | ||
| logger.log(`${step}: ${status}`); | ||
| }, | ||
| }; | ||
|
|
||
| if (imperativeManager.hasDeployersForPhase('post-cdk', teardownContext)) { | ||
| logger.startStep('Tear down imperative resources'); | ||
| const teardownResult = await imperativeManager.teardownAll(teardownContext); | ||
| if (!teardownResult.success) { | ||
| logger.endStep('error', teardownResult.error); | ||
| throw new Error(`Imperative teardown failed: ${teardownResult.error}`); | ||
| } | ||
| logger.endStep('success'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // After deploying the empty spec, destroy the stack entirely | ||
| const targetName = context.awsTargets[0]?.name; | ||
| if (targetName) { | ||
|
|
||
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.
This changes the e2e tests on pr so that if any helper files change it runs all the tests on the pr. This is important as when people are changing tests we want to see the changes for the tests.