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

CMDCT-4238 - Prevent destroy from destroying serverless stacks unless particular resources are set to retain #15026

Merged
merged 12 commits into from
Jan 14, 2025

Conversation

JonHolman
Copy link
Contributor

@JonHolman JonHolman commented Jan 9, 2025

Description

Idea to avoid accidentally deleting important resources during the CDK migration

Related ticket(s)

CMDCT-4238


How to test

Notes


Pre-review checklist

  • I have added thorough tests, if necessary
  • I have updated relevant documentation, if necessary
  • I have performed a self-review of my code
  • I have manually tested this PR in the deployed cloud environment

Pre-merge checklist

Review

  • Design: This work has been reviewed and approved by design, if necessary
  • Product: This work has been reviewed and approved by product owner, if necessary

Security

If either of the following are true, notify the team's ISSO (Information System Security Officer).

  • These changes are significant enough to require an update to the SIA.
  • These changes are significant enough to require a penetration test.

convert to a different template: test → val | val → prod

@JonHolman JonHolman marked this pull request as ready for review January 14, 2025 18:37
src/run.ts Outdated
Comment on lines 221 to 246
const resourcesToCheck = [
{
templateKey: `database-${options.stage}`,
resourceKey: "FormAnswersTable",
},
{
templateKey: `database-${options.stage}`,
resourceKey: "FormQuestionsTable",
},
{
templateKey: `database-${options.stage}`,
resourceKey: "FormTemplatesTable",
},
{ templateKey: `database-${options.stage}`, resourceKey: "FormsTable" },
{
templateKey: `database-${options.stage}`,
resourceKey: "StateFormsTable",
},
{ templateKey: `database-${options.stage}`, resourceKey: "StatesTable" },
{ templateKey: `database-${options.stage}`, resourceKey: "AuthUserTable" },
{
templateKey: `ui-${options.stage}`,
resourceKey: "CloudFrontDistribution",
},
{ templateKey: `ui-auth-${options.stage}`, resourceKey: "CognitoUserPool" },
];

Choose a reason for hiding this comment

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

You'd have to change some other code I'm sure but I wonder if you like this stripped down version of this same data:

const resourcesToCheck = {
  `database-${options.stage}`: "FormAnswersTable",
  `database-${options.stage}`: "FormQuestionsTable",
  `database-${options.stage}`: "FormTemplatesTable",
  `database-${options.stage}`: "FormsTable"
  `database-${options.stage}`: "StateFormsTable",
  `database-${options.stage}`,: "StatesTable"
  `database-${options.stage}`:  "AuthUserTable"
  `ui-${options.stage}`: "CloudFrontDistribution",
  `ui-auth-${options.stage}`: "CognitoUserPool" 
}

src/run.ts Outdated Show resolved Hide resolved
src/run.ts Outdated
);

if (
["master", "val", "production"].includes(options.stage) &&

Choose a reason for hiding this comment

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

Any way to make this match isDev's definition? Also if that's complicated feel free to ignore.

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 isn't in the cdk branches, this will be in sls branches, so we don't have isDev.

peoplespete
peoplespete previously approved these changes Jan 14, 2025
Copy link

@peoplespete peoplespete left a comment

Choose a reason for hiding this comment

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

A few things to consider but when you're happy I'm happy

Co-authored-by: Pete Dunlap <[email protected]>
Copy link

codeclimate bot commented Jan 14, 2025

Code Climate has analyzed commit 4df86bf and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (90% is the threshold).

This pull request will bring the total coverage in the repository to 82.1% (0.0% change).

View more on Code Climate.

Copy link

@peoplespete peoplespete left a comment

Choose a reason for hiding this comment

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

Chatted in slack, ready when you are

@JonHolman JonHolman changed the base branch from master to pete-sls January 14, 2025 20:45
@JonHolman JonHolman merged commit 5bd8e40 into pete-sls Jan 14, 2025
15 checks passed
@JonHolman JonHolman deleted the jon4 branch January 14, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants