refactor(cli): cleanup of CLI code #7041
Conversation
More CLI refactorings in preparation for "convention mode" deployments. * Move remaining routines from `cdk.ts` (the `main()` function) into the `CdkToolkit` class so that the implementation of all CLI functionality lives in the same place. * `ISDK` now can be interrogated about its "current" account/region (which are different from the `SdkProvider`s "default" account/region). * Split `AppStacks` into 2 distinct classes: `CloudExecutable`, responsible for producing a `CloudAssembly`, which contains routines for operating upon the cloud assembly (selecting stacks, processing error messages). * Get rid of many repeated low-level CloudFormation `DescribeStacks` (et. al.) API calls by folding them into a `CloudFormationStack` object which contains (and can be interrogated about) the state of a deployed stack. * CloudFormation parameters: treat parameters as a map of string->string for as long as possible, translating only to API objects at the very last moment. This simplifies functions dealing with CloudFormation parameters all-around. * Get rid of the premature abstraction of `class CloudFormationTarget implements IDeploymentTarget`, demote it to a simple CloudFormation helper class called `CloudFormationDeployments` (its responsibilities are pretty weak right now and it could probably be removed, but for now I didn't do that). * Turn `loadToolkitInfo` into `ToolkitInfo.lookup()` which is a nice idiom we should be using in more places.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
skinny85
left a comment
skinny85
left a comment
There was a problem hiding this comment.
I am OK with this. I don't feel like I have enough knowledge of this part of the codebase to comment much on the specifics.
I've added the 'do not automatically merge' label to give the other reviewers a chance to chime in.
shivlaks
left a comment
shivlaks
left a comment
There was a problem hiding this comment.
looks good to me... a few minor things and some questions around things you've touched but not necessarily changed :)
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
shivlaks
left a comment
shivlaks
left a comment
There was a problem hiding this comment.
I'm good with this round of refactoring. Apologies in advance as you'll likely have to merge in the stack outputs changes :)
|
The build timed out :( |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
1 similar comment
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
The automatic parameter reuse which was introduced in #7041 has been causing issues for some people who use Default Values to classify between CI/CD deployments and CLI deployments (value supplied=CI/CD, value default=CLI). Introduce a switch to turn it off.
The automatic parameter reuse which was introduced in #7041 has been causing issues for some people who use Default Values to classify between CI/CD deployments and CLI deployments (value supplied=CI/CD, value default=CLI). Introduce a switch to turn it off.
The automatic parameter reuse which was introduced in aws#7041 has been causing issues for some people who use Default Values to classify between CI/CD deployments and CLI deployments (value supplied=CI/CD, value default=CLI). Introduce a switch to turn it off.
4 participants
Sorry for another monster commit :(
If desired I will try to send some effort next week trying to split it.
Commit Message
More CLI refactorings in preparation for "convention mode" deployments.
cdk.ts(themain()function) into theCdkToolkitclass so that the implementation of all CLI functionality lives in the same place.ISDKnow can be interrogated about its "current" account/region (which are different from theSdkProviders "default" account/region).AppStacksinto 2 distinct classes:CloudExecutable, responsible for producing aCloudAssembly, which contains routines for operating upon the cloud assembly (selecting stacks, processing error messages).DescribeStacks(et. al.) API calls by folding them into aCloudFormationStackobject which contains (and can be interrogated about) the state of a deployed stack.class CloudFormationTarget implements IDeploymentTarget, demote it to a simple CloudFormation helper class calledCloudFormationDeployments(its responsibilities are pretty weak right now and it could probably be removed, but for now I didn't do that).loadToolkitInfointoToolkitInfo.lookup()which is a nice idiom we should be using in more places.End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license