-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add drift detection to cdk as cdk drift
#442
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
base: main
Are you sure you want to change the base?
Conversation
Thanks! This is a great feature to have! I understand that we're detecting some kind of "diff" from a user point of view, but I'm very put-off by drift detection being part of
Can we make this its own subcommand to begin with?
Surely there's more? 🥹 An integ test would be nice. |
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.
Also needs integ tests and added to toolkit-lib
(see Rico's comments).
If we are adding this as a new action/command, the action code should live in toolkit-lib and CLI just be calling it.
@rix0rrr Let's discuss this further during Tuesday's meeting. While I'm open to making this a separate command, I'd like to explain my original reasoning for including it in I envisioned
However, I recognize your point about the historical context of |
In favour of using the SDK's enum value, which CFN uses anyways
I see that, and I like the idea of a general There is some precedent for having certain features both as their own commands as well as options to other commands. So I would propose to have both:
|
came here to say this. |
await ioHelper.notify(IO.CDK_TOOLKIT_I4506.msg('Waiting for drift detection to complete...')); | ||
checkIn = Date.now() + timeBetweenOutputs; | ||
} | ||
await new Promise(resolve => setTimeout(resolve, 500)); |
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 seems like it's doing nothing?
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.
It's meant to give a brief half-second delay between iterations. Without this, it overflows the debug output with messages every chance it gets
const output = []; | ||
|
||
for (const stack of stacks.stackArtifacts) { | ||
const cfn = (await (await this.sdkProvider('drift')).forEnvironment(stack.environment, Mode.ForReading)).sdk.cloudFormation(); |
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.
Can this be outside the loop?
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.
I don't believe so, since this will do the drift detection on each stack in the names provided.
Fixes # N/A
Added an additional command,
cdk drift
, which invokes drift detection. Prints an output of the drift results listing out any resources in the stack that may have drifted.Tested via
yarn build && yarn test
and adding in a few integ tests.Running the command locally shows the following. With one resource drifted:

With no resources drifted: (image outdated)

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