Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(cli): CDK Migrate CLI command #27325

Merged
merged 27 commits into from
Oct 6, 2023
Merged

feat(cli): CDK Migrate CLI command #27325

Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5091192
add CDK migrate readme
Sep 27, 2023
ab21a14
modifications to cdk migrate readme
Sep 27, 2023
9c9002c
some fixes to readme, still WIP
Sep 27, 2023
659fb6f
update readmea with more info
Sep 28, 2023
db94dc7
remove random whitespace
Sep 28, 2023
f21ee4f
added more intro to the cdk migrate command
Oct 2, 2023
94d2f39
readme modifications
Oct 2, 2023
681b88d
change some wording
Oct 2, 2023
50f4b6d
remove description of flags
Oct 3, 2023
64623c4
Merge branch 'main' into bobertzh/docs
HBobertz Oct 3, 2023
d8052cf
annotate code block with json
Oct 3, 2023
c5c0d1a
callout the L1s
Oct 3, 2023
81ed49f
updtae to readme
Oct 3, 2023
a13630b
Update packages/aws-cdk/README.md
HBobertz Oct 4, 2023
cb94039
Update packages/aws-cdk/README.md
HBobertz Oct 4, 2023
06e0b88
Update packages/aws-cdk/README.md
HBobertz Oct 4, 2023
ff41695
update readme with comments
Oct 4, 2023
c884f58
added a limitations table
Oct 4, 2023
9327161
Merge branch 'main' into bobertzh/docs
HBobertz Oct 4, 2023
7087358
update readme to have bullets this time
Oct 5, 2023
8c8e37c
change * to -
Oct 5, 2023
806d542
Merge branch 'main' into bobertzh/docs
HBobertz Oct 5, 2023
eb1c0e4
more readme updates
Oct 5, 2023
c02bb96
more updates
Oct 5, 2023
7af64c5
Merge branch 'main' into bobertzh/docs
HBobertz Oct 5, 2023
3104733
Merge branch 'main' into bobertzh/docs
HBobertz Oct 6, 2023
6cae5ff
Merge branch 'main' into bobertzh/docs
madeline-k Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/aws-cdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,32 @@ NOTICES
If you don’t want to see a notice anymore, use "cdk acknowledge <id>". For example, "cdk acknowledge 16603".
```

### `cdk migrate`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is under the "Notices" section. Can you move it up to the "Commands" section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't CDK notices the command?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. there is a section header for Notices with cdk notices and cdk acknowledge both under it. If you view this file as rendered markdown, it is clear that this command is not in the right section.


⚠️**CAUTION**⚠️ CDK migrate is currently experimental. We make no guarantees about the outcome or stability of the Command or it's output.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Experimental means that it might have breaking changes in the future. Saying we make no guarantees about the commands output sounds like the command may or may not do what the documentation says, and that is not the case.

Reusing the exact phrasing from another experimental command:

Suggested change
⚠️**CAUTION**⚠️ CDK migrate is currently experimental. We make no guarantees about the outcome or stability of the Command or it's output.
⚠️**CAUTION**⚠️ This command is considered experimental, and might have breaking changes in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we currently output when a user runs our command. Should we change it in both places?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be in favor of that, yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
⚠️**CAUTION**⚠️ CDK migrate is currently experimental. We make no guarantees about the outcome or stability of the Command or it's output.
⚠️ **CAUTION** ⚠️
CDK migrate is currently experimental. We make no guarantees about the outcome or stability of the Command or it's output.


Generates a CDK application from an existing Cloudformation template in any of the CDK supported languages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Generates a CDK application from an existing Cloudformation template in any of the CDK supported languages.
Generates a CDK application from an existing CloudFormation template in any of the CDK supported languages.

Required Arguments:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In md, this will be inlined with the following lines, and not look like a title.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a paragraph above this that explains in more detail what the command is and how to use it? Think about how you would teach someone who is new to CDK how to use this command to get started with CDK.

* `--from-path <my_file_path>` - Takes the relative filse path to the JSON or YAML template of your choice
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolute paths should also work.

Suggested change
* `--from-path <my_file_path>` - Takes the relative filse path to the JSON or YAML template of your choice
* `--from-path <my_file_path>` - The file path to the JSON or YAML CloudFormation template

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `--from-path <my_file_path>` - Takes the relative filse path to the JSON or YAML template of your choice
* `--from-path <my_file_path>` - Takes the relative file path to the JSON or YAML CloudFormation template

* `--stack-name <my_stack_name>` - Used to name both the CDK application and stack.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is it used? Is it used verbatim? Is there a transformation that's applied? Is the CDK App name === the stack name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point the CDK App name === the stack name. We want to change this later. I'll make this more explicit

Optional Arguments:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supposed to be a title - but will show inline and this will all be one paragraph.

* `--output-path <my_output_path>` - file path to where the file should be generated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `--output-path <my_output_path>` - file path to where the file should be generated.
* `--output-path <my_output_path>` - file path where the application will be generated

Default is the current working directory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file? What type of file? Initially I thought this was supposed to be the generated CDK app, which is a folder (or maybe a zip file), but now I'm thinking it means the stack.ts file. If so it should say something like:

* `--output-path <my_output_path>` - path where the generated CDK Stack will be located.
  Default is `./foo.ts`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unclear by me. Fixing

* `--language <language>` - Which CDK supported language should be generated [typescript, python, csharp, java, go]
default is typesript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default is typesript
default is typescript

```console
$ # generate a typescript application from template.json in the local directory
$ cdk migrate --from-path ./template.json --stack-name MyAwesome

$ # generate a python application from template.json in the local directory
$ cdk bootstrap migrate --from-path ./template.json --stack-name MyAwesome
```

In order for the generated stack to be immediatly deployable, that stack must either already exist in that region/account
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put a header above this that says "Limitations" like other commands have done (import)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this section is pretty confusing, but I am struggling to come up with better phrasing here. I imagine customers will read this and ask, what does it mean to be immediately deployable? How do I know if there is any overlap between existing resources and resources defined in the input template?

An idea to improve clarity would be to separate this into describing the two use cases that work: 1. deploying to an environment with a deployed stack that matches the input template. 2. deploy to an environment with no deployed resources that overlap with your template definition.
Then finally, include the limitations section saying if you try to deploy a stack generated by CDK migrate to an environment that meets any of the following criteria, deployment will fail.

  • resources with the same physical ids already exist
  • a partially-matching stack already exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In order for the generated stack to be immediatly deployable, that stack must either already exist in that region/account
In order for the generated application to be immediately deployable, that stack must either already exist in that region/account

with the exact same configuration of resources, or not exist in that account/region at all. This effectively means
you need to get the template you are using from a deployed stack and use it in that region, or use it in a new one
that wont have any overlap with deployed resources

### Bundling

By default asset bundling is skipped for `cdk list` and `cdk destroy`. For `cdk deploy`, `cdk diff`
Expand Down
Loading