feat(gen2-migration): cfn drift detection by 9pace · Pull Request #14303 · aws-amplify/amplify-cli

9pace · 2025-10-28T15:14:14Z

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)
New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Resolves merge conflicts between drift detection functionality and gen2-migration branch. Key conflicts resolved: - drift.ts: Combined drift detection implementation with gen2-migration structure - package.json files: Merged dependency changes - gen2-migration files: Preserved gen2-migration functionality - yarn.lock: Resolved dependency conflicts

iliapolo · 2025-11-05T20:14:00Z

+        // Parse and format messages based on their content
+        if (msg.includes('Found') && msg.includes('nested')) {
+          // Extract indentation, count, and type
+          const indent = msg.match(/^(\s*)/)?.[1] || '';


To fix the issue, we should remove the unused variable declaration at line 133 which reads the indentation from the incoming message string: const indent = msg.match(/^(\s*)/)?.[1] || '';. This statement is unnecessary and can be safely deleted. The rest of the block will continue to work correctly, as none of the output or logic depends on indent. Only this single variable declaration (the line itself) should be changed; all surrounding code should be left intact.

Copilot is right.

iliapolo · 2025-11-05T20:14:09Z

+          // Extract indentation, count, and type
+          const indent = msg.match(/^(\s*)/)?.[1] || '';
+          const count = msg.match(/Found (\d+)/)?.[1] || '0';
+          const level = msg.match(/level (\d+)/)?.[1];


To resolve the unused variable warning, delete the declaration and assignment of the level variable on line 135 within the createPrintObject function. Do not alter the rest of the logic: keep processing indent and count as before, since they are being used. Remove only level and its assignment, ensuring that indentation and code layout are preserved.

iliapolo · 2025-11-05T19:16:46Z

+  const exitCode = await detector.detect(options);
+
+  if (exitCode !== 0) {
+    process.exitCode = exitCode;


Prefer throwing an error (AmplifyError?) to make it clearer to the user the command failed

iliapolo · 2025-11-05T19:18:17Z

  },
  "packageManager": "yarn@3.5.0",
  "resolutions": {
+    "@aws-sdk/client-cloudformation": "^3.624.0",


Why is this necessary?

iliapolo · 2025-11-05T19:21:36Z

+import chalk from 'chalk';
+import { detectStackDriftRecursive, type DriftDisplayFormat } from './drift-detection';
+import { CloudFormationService, AmplifyConfigService, FileService, DriftFormatter } from './drift-detection/services';
+import type { CloudFormationClient } from '@aws-sdk/client-cloudformation';


iliapolo · 2025-11-05T19:21:51Z

+import { $TSContext, AmplifyError } from '@aws-amplify/amplify-cli-core';
+import { printer } from '@aws-amplify/amplify-prompts';
+import chalk from 'chalk';
+import { detectStackDriftRecursive, type DriftDisplayFormat } from './drift-detection';


DriftDisplayFormat unused

iliapolo · 2025-11-05T20:04:31Z

+
+    // Display initial status
+    printer.info('');
+    printer.info(chalk.cyan.bold(`Started Drift Detection for Project: ${projectName}`));


We should also say which environment we are checking

iliapolo · 2025-11-05T21:21:02Z

+            }
+          }
+        } catch (e) {
+          // If parsing fails, log and use the original value


If we are good with both a name and an ARN - why choose over the other?

iliapolo · 2025-11-05T21:22:01Z

+
+      // Validate the extracted name
+      if (!nestedStackName || nestedStackName === nestedStack.PhysicalResourceId) {
+        logger.logInfo({


Why validate if we are just logging?

iliapolo · 2025-11-05T21:22:46Z

+      }
+
+      // Validate the extracted name
+      if (!nestedStackName || nestedStackName === nestedStack.PhysicalResourceId) {


Why not just use nestedStack.PhysicalResourceId and call it a day? what scenario are you trying to protect against?

iliapolo · 2025-11-05T21:24:27Z

+    }
+  }
+
+  logger.logInfo({


When are these being printed to screen? with --verbose?

iliapolo · 2025-11-05T21:25:18Z

+        message: `detectStackDriftRecursive.nestedStack: ${nestedStack.LogicalResourceId}, ${nestedResults.rootStackDrifts.StackResourceDrifts?.length} direct resources, ${nestedResults.nestedStackDrifts.size} sub-stacks`,
+      });
+    } catch (error: any) {
+      // Log error but continue checking other nested stacks


Yeah but again in the context of migration I would prefer this to fail - to be extra safe.

iliapolo and others added 12 commits October 8, 2025 22:18

chore: scaffolding migration

93f57e4

chore: scaffolding migration

55c8670

feat: cfn drift detection base

ad39bf7

fix: find root stack from meta instead of team-provider-info

107565c

fix: refactor to cli package

15f1208

feat: implement nested stack traversal and pretty tree formatting

eb5d2ab

fix: handle cfn drift check failure with UNKNOWN status

89ea369

chore: refactored redundant drift-formatter code

19b5ea4

feat: services structure refactor and recurse correctly

56a1460

feat: arn parsing and add drift-formatter to services

29059de

refactor: consolidate formatting to refactored drift-formatter

66b86e5

9pace marked this pull request as ready for review November 3, 2025 19:56

9pace requested a review from a team as a code owner November 3, 2025 19:56

chore: remove eroneous refactor.ts file

c11e876

github-advanced-security AI found potential problems Nov 3, 2025

View reviewed changes

chore: remove unecessary comment from old code

5c5f382

iliapolo requested changes Nov 5, 2025

View reviewed changes

iliapolo approved these changes Nov 7, 2025

View reviewed changes

iliapolo merged commit 8366503 into gen2-migration Nov 7, 2025
4 checks passed

iliapolo deleted the drift-detection branch November 7, 2025 13:41

9pace restored the drift-detection branch November 12, 2025 16:38

Conversation

9pace commented Oct 28, 2025

Description of changes

Issue #, if available

Description of how you validated changes

Checklist

Uh oh!

Check notice

Copilot Autofix

Choose a reason for hiding this comment

Uh oh!

Check notice

Copilot Autofix

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants