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.

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(core): allow setting termination protection on stacks #14719

Closed
wants to merge 3 commits into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 16, 2021

This PR adds a getter and setter for Stack termination protection. This allows termination protection to be configured after the Stack has been constructed.

fixes #14463

This PR also includes a commit that fixes several linter warnings in stack.ts.


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

cjihrig added 2 commits May 15, 2021 21:17
This commit silences several linter warnings in the stack.ts file.
This commit adds a getter and setter for Stack termination
protection. This allows termination protection to be configured
after the Stack has been constructed.

fixes aws#14463
@gitpod-io
Copy link

gitpod-io bot commented May 16, 2021

@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label May 16, 2021
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: f59dad0
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@NetaNir NetaNir left a comment

Choose a reason for hiding this comment

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

We don't normally use getter and setter in the public API (searching for public set in the codebase returns very few results). The reason behind it that for values known at construction time, this added mutability does not adds value in terms of API ergonomics.
If there is a use case Im missing, let me know.

@NetaNir NetaNir assigned NetaNir and unassigned rix0rrr May 19, 2021
@cjihrig
Copy link
Contributor Author

cjihrig commented May 19, 2021

The reason behind it that for values known at construction time, this added mutability does not adds value in terms of API ergonomics.

Are you saying that #14463 is a won't fix, or you'd just prefer a different implementation here? If you'd like a different implementation here, I'm open to update this however you'd like.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 12, 2021

Ping @NetaNir regarding my previous comment ^

@cjihrig cjihrig requested a review from NetaNir June 20, 2021 00:51
@NetaNir NetaNir removed their assignment Jun 22, 2021
@cjihrig cjihrig closed this Aug 24, 2021
@rittneje
Copy link

The reason behind it that for values known at construction time, this added mutability does not adds value in terms of API ergonomics. If there is a use case Im missing, let me know.

@NetaNir The use case is described in my original issue that @cjihrig linked. In general, if you don't allow fields to be mutable after construction, that basically rules out many use cases for Aspects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(core): add setter for termination protection
5 participants