rfc: bring back removed cdk bootstrap CLI options

[skinny85]

In the original RFC for the modified cdk bootstrap command, we removed two options from the cdk bootstrap command-line utility: --toolkit-bucket-name and --bootstrap-kms-key-id. In this small revision to that RFC, I propose we not remove them to preserve backwards compatibility for customers who already use those options, and I show what changes that would require to our bootstrap CloudFormation template.

---

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

[mergify]

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[eladb]

I am okay with allowing to customize the KMS key because it's not referenced by the app but I have a slight reservation about allowing customization of the bucket name because specifying a custom bucket name during bootstrapping will not "just work". It also requires that users will override the asset behavior at the Stack-level to provide a different assets bucket name.

Remember that the asset publishing tool doesn't know anything about bootstrapping. It simply gets the S3 location to upload the file, so the error it can emit is something like "bucket not found". We can add a hint there that says "please make sure your environment has been bootstrapped to include the bucket BUCKET_NAME" or something like that.

Furthermore, if we allow specifying the bucket name, we should also allow specifying the ECR repository name. It doesn't make sense to only support one of them.

There are also other degrees of freedom that users may want to introduce like adding IAM boundaries for example to the roles, but I really didn't want to get into the business of offering all the degrees of freedom in the bootstrapping template (it's way over-complicated as it is). I rather this template be a simple and straightforward template which users can easily fork and modify as oppose to making it very complex and hard to customize.

[skinny85]

Here's my take on this.

I think if we were designing bootstrapping, and its CLI options, today, with the knowledge about the CI/CD story for CDK apps that we have now, we would probably make different decisions than we did originally.

But given that we are where we are currently, I think trying to preserve backwards compatibility is worth it. The fact that we have these options at all indicates to me there is a need for these sort of customizations. Removing CLI options also adversely affects the customer's trust in the CDK itself.

I understand we will have some challenges that we didn't face before, but I feel like we can produce good enough messages, errors and docs (which we will have to do anyway, regardless if we agree to have these degrees of freedom or not, as this is a very complex feature we're planning) that it will be at least clear to the customer where the problem is coming from, and how to unblock themselves.

I like your idea about the ECR repo - we don't have to expose a CLI option for it yet, but it makes sense to be prepared for if/when we do. I'll add it.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[eladb]

My point is that even if we leave bucket name as an option, we will be breaking the current behavior. Today if I specify a bucket name for bootstrapping, I can simply start deploying apps to this environment. Tomorrow, it will fail.

But ok, let’s leave this but also make sure that if you are not using the default “CDK bootstrap” prints a message with instructions on how to configure the asset bucket name when the stack is defined.

And also, if we leave this we must also add support for specifying the ECR repo name. It doesnt make sense to support one without the other.

[skinny85]

My point is that even if we leave bucket name as an option, we will be breaking the current behavior. Today if I specify a bucket name for bootstrapping, I can simply start deploying apps to this environment. Tomorrow, it will fail.

But ok, let’s leave this but also make sure that if you are not using the default “CDK bootstrap” prints a message with instructions on how to configure the asset bucket name when the stack is defined.

Yep, that's the plan.

And also, if we leave this we must also add support for specifying the ECR repo name. It doesnt make sense to support one without the other.

Agreed, and it has already been done in this commit: 816a7c8 that's part of this PR as well.

[eladb]

@skinny85 what's up with this? Should we close?

[skinny85]

@skinny85 what's up with this? Should we close?

Still waiting for your and @rix0rrr 's approval for these changes.

[eladb]

Do we need the "Custom" prefix?

[skinny85]

Yes, the "Custom" prefix has since been removed. Let me rebase this PR on top of the latest master (in master, this parameter is simply called ContainerAssetsRepositoryName).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 9ca0262
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository