rfc: changes to default stack environments #4131
Conversation
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| - When `cdk synth` prints to STDOUT is prints in YAML (unless `--json` is specified), while `cdk.out` always uses JSON ([#2965](https://github.com/aws/aws-cdk/issues/2965)). | ||
| - `cdk.out` is created no matter what, for both use cases of `synth` and even if it the app was synthesized implicitly as part of `cdk deploy`. This also somewhat causes confusion. | ||
|
|
||
| We propose to separate the "synth" use cases into two different CLI commands: `cdk print` and `cdk synth`. The first will serve the inspection use case and the second the CI/CD use case. This way, we can decide that `--default-env` will have a different default behavior for these two commands. For `cdk print` we will default to `agnostic` and for `cdk synth` we will default to `explicit`. |
There was a problem hiding this comment.
I am worried that people would want to use cdk print (I don't love the name here, BTW) to check what will be deployed, and that they will be surprised that what is synth'd and deploy'd is not the same...
To me, literally the only reason why you'd want to "see the template" is to troubleshoot or validate what'll be deployed before you go do it... If the default behaviors aren't the same, then this will be surprising.
There was a problem hiding this comment.
I wonder if we even need the print subcommand. A combination of good output messaging on cdk synth (like "synthesized stackA at ") and cat/less/vim/etc. should work well just fine here.
That would also mean that synth acknowledge the --json (or yaml) option and render the stack under cdk.out appropriately.
There was a problem hiding this comment.
I would comment that the default environment for cdk deploy and cdk print would need to be the same for print to be useful (if it is intended for debugging). However, IF it is intended for debugging, we'd be better off calling it something else cdk debug? cdk troubleshoot? Or make it a flag to synth: cdk synth --as-deployed.
|
After reading it, my first impression was that there are far too many options which makes it very complex. How am I supposed to explain this to someone who has little or no knowledge of CDK? It get's even more complicated if you think about the "lookupFromXXX()" methods. And how does the |
|
@pgarbe yes, this could be quite confusing, but the purpose here is to actually make the experience more seamless. For example, at the moment if you want to use But I hear you... And I agree with @RomainMuller's point that it might be confusing for people that use |
|
@eladb Yes, I agree that the current situation with |
eladb
changed the title
There was a problem hiding this comment.
Though I agree that the number of options creates some complexity, it is my feeling that this complexity exists within the underlying tools (aws-cli, cloudformation, etc) with regards to regions. Overall I believe this proposal would indeed make usage more intuitive for the synth and deploy workflows for the default case (not passing a --default-env).
Separation of the print command also makes sense to me. A common alternative is using an option --dry-run that when enabled would print the template to stdout instead of creating .cdk.out/. For those that believe that synth should cover the same use case this may make more sense.
| The proposal is: | ||
|
|
||
| 1. Add an `--default-env=inherit|explicit|agnostic` switch to the CLI which will control the default env mode. | ||
| 2. Extend the semantics of `CDK_DEFAULT_ACCOUNT` and `CDK_DEFAULT_REGION` to be able to express these three modes (currently it cannot express environment-agnosticness). |
There was a problem hiding this comment.
This is a vague sentence. Does that mean the values in those environment variables will be different? What do you mean here?
| 3. Change the default behavior of `env` in `Stack` to fall back to `CDK_DEFAULT_ACCOUNT` and `CDK_DEFAULT_REGION` if an explicit value is not specified. | ||
| 4. If `env` is not explicitly set and `CDK_DEFAULT_XXX` is undefined, stack initialization **will fail with an error**. | ||
| 5. The default mode for `cdk deploy` will be `inherit`. This is because when a stack without env is deployed with the CLI, we *know* where it's going. There's no point playing this game here. | ||
| 6. The default mode for `cdk synth` will be `explicit`. The implication of this (detalied below) is that if users wish to synthesize an artifact from their CDK app and deploy it at a later stage, they will have to either explicitly specify `env` for all stacks or use `cdk synth --default-env=inherit|agnostic`. |
There was a problem hiding this comment.
I find this dangerous, and we should not treat the behavior change lightly. I think people have been trained to look at cdk synth to determine what their deployed stack will look like. I know I do.
We HAVE TO commit to clearly documenting/explaining how you troubleshoot your deployment (look at the generated template).
I would also say we have to have cdk synth detect whether a cdk deploy would deploy something else and printing something like:
The stack you see above is environment-agnostic and may not be what will be deployed when you run `cdk deploy`.
See https://docs.aws.amazon.com/.../troubleshooting.html
There was a problem hiding this comment.
Also the "environments" section of the developer guide will need be rewritten. Ping @jerry-aws .
| - When `cdk synth` prints to STDOUT is prints in YAML (unless `--json` is specified), while `cdk.out` always uses JSON ([#2965](https://github.com/aws/aws-cdk/issues/2965)). | ||
| - `cdk.out` is created no matter what, for both use cases of `synth` and even if it the app was synthesized implicitly as part of `cdk deploy`. This also somewhat causes confusion. | ||
|
|
||
| We propose to separate the "synth" use cases into two different CLI commands: `cdk print` and `cdk synth`. The first will serve the inspection use case and the second the CI/CD use case. This way, we can decide that `--default-env` will have a different default behavior for these two commands. For `cdk print` we will default to `agnostic` and for `cdk synth` we will default to `explicit`. |
There was a problem hiding this comment.
I would comment that the default environment for cdk deploy and cdk print would need to be the same for print to be useful (if it is intended for debugging). However, IF it is intended for debugging, we'd be better off calling it something else cdk debug? cdk troubleshoot? Or make it a flag to synth: cdk synth --as-deployed.
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
3 similar comments
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
1 similar comment
|
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Originally, we planned to disallow the use of assets by environment-agnostic stacks (e.g. stacks that are defined with pseudo ACCOUNT/REGION in `env`). The problem is that today this is the default behavior for `new Stack()`, which obviously can't work. The original plan was to modify the default behavior of `env` (see aws/aws-cdk#4131), and we still want to pursue this, but since this change in default behavior has potential risk involved, we wanted to decouple it from our CI/CD delivery. Therefore, this change updates the CI/CD and cdk-assets RFCs to support assets in environment-agnostic stacks by allowing the use `${AWS::Region}` and `${AWS::AccountId}` in `assets.json`.
|
Since this is not in the critical path for CI/CD, I am closing for now and we will revisit soon |
We are looking to change the default behavior for when a
Stackis defined withoutenvin order to improve the developer experience.When using
cdk deploydirectly with an app that has stack(s) without explicitenv, we can inherit the CLI's current environment instead of synthesize environment-agnostic stacks.Comments are welcome!
Rendered view: here
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license