-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(all): migrate to CDKv2 #738
Conversation
})); | ||
}); | ||
|
||
test('creates an asset', () => { |
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.
Note: This test is not possible with the under-the-hood changes introduced in CDKv2. Fortunately, this is tested, in a way, by the tests that look at UserData; the CDK now generates the file sha256 before synthesis, so file hashes are directly embedded into UserData in both the rendered stack and when directly rendering the UserData string from the contruct.
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.
Great job, especially with making all tests work.
Just a couple moment I want to clarify.
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/config.py
Outdated
Show resolved
Hide resolved
examples/deadline/All-In-AWS-Infrastructure-Basic/python/package/lib/storage_tier.py
Show resolved
Hide resolved
3158b4e
to
fd2ee20
Compare
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.
LGTM! Thanks for doing this migration. Just found a couple bits of commented out code, but not holding back the PR for it
//const res = Template.fromStack(stack).findResources('AWS::IAM::Policy', Match.anyValue); | ||
//expect(res).toBe({ foo: 'bar' }); |
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.
nit: commented out code
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.
Drat. Will fix. Thanks!
Tags, | ||
} from '@aws-cdk/core'; | ||
} from 'aws-cdk-lib/aws-iam'; | ||
//import { ArtifactMetadataEntryType } from '@aws-cdk/cloud-assembly-schema'; |
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.
nit: commented out code
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.
Drat. Will fix. Thanks!
* Migrates the RFDK library, examples, and integration tests to CDKv2. BREAKING CHANGE: This change migrates the RFDK to be based on CDK v2. All apps that use the RFDK will have to be migrated to CDKv2 as well. To update your app, follow the CDK's migration guide at: https://docs.aws.amazon.com/cdk/v2/guide/migrating-v2.html
fd2ee20
to
db96f35
Compare
BREAKING CHANGE: This change migrates the RFDK to be based on CDK v2.
All apps that use the RFDK will have to be migrated to CDKv2 as well.
To update your app, follow the CDK's migration guide at:
https://docs.aws.amazon.com/cdk/v2/guide/migrating-v2.html
Some notes:
@aws-cdk/asserts
module that we were using for unit tests and replaced it withaws-cdk-lib/assertions
. This was a major change. Syntax and functionality changed. The CDK team provides a migration guide (here: https://github.com/aws/aws-cdk/blob/v1-main/packages/@aws-cdk/assertions/MIGRATING.md ) that includes a migration script; the script got about 70% of the way, and the rest had to be done manually. The biggest change was that once you've asserted properties of a stack/app in a test then the state of the stack is baked-in at that time. So, you cannot, say, assert a property, then add something to the stack, and then assert a new property -- the second assertion will fail as it will be asserting against the stack pre-addition; this change required retooling some unit tests.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license