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

fix(redshift): tables were dropped on table name change #24308

Merged
merged 56 commits into from
Dec 7, 2023
Merged

fix(redshift): tables were dropped on table name change #24308

merged 56 commits into from
Dec 7, 2023

Conversation

Rizxcviii
Copy link
Contributor

@Rizxcviii Rizxcviii commented Feb 23, 2023

Upon a refactor of the UserTablePrivilieges now using tableId to record changes, this fix will essentially allow table names to be changed.

Closes #24246.

BREAKING CHANGE: Further updates of the Redshift table will fail for existing tables, if the table name is changed. Therefore, changing the table name for existing Redshift tables have been disabled.


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

@gitpod-io
Copy link

gitpod-io bot commented Feb 23, 2023

@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Feb 23, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team February 23, 2023 16:16
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Feb 23, 2023
@Rizxcviii Rizxcviii marked this pull request as draft February 23, 2023 22:57
@Rizxcviii Rizxcviii marked this pull request as ready for review February 24, 2023 15:48
@Rizxcviii Rizxcviii marked this pull request as draft February 24, 2023 20:01
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation dismissed their stale review March 1, 2023 14:46

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 9, 2023
@scanlonp
Copy link
Contributor

@Rizxcviii thanks for all of your responses! I will take another pass over the code with your comments & updates in mind, but I felt pretty good about it before. Since this module is still in alpha, I do not believe we will need a feature flag; we can just go with the new default behavior. But I will put some more thought into it.

My last request is confirmation that the update functionality works as intended. I put some more details in the integ test comment from my last review (essentially our testing cannot handle update behavior)!

@Rizxcviii
Copy link
Contributor Author

Rizxcviii commented Nov 16, 2023

@scanlonp after testing locally, I discovered a fault. Which was assuming Data attributes are sent across requests. This is not the case, and there does not seem to be a way to move tableName between deployments other than through the OldResourceProperties object.

Given that you do not feel we need a feature flag for this construct, I can continue to work on this change. The caveat is previously created tables will no longer be able to update their tables, if they were to change their table name.

@scanlonp
Copy link
Contributor

@Rizxcviii, glad you found that!
So the new behavior will be that if someone has an existing table, and they redeploy with a new name, that table will no longer be able to be updated?
Any this does not apply to any other scenario? i.e. newly created table -> change table name?

@Rizxcviii
Copy link
Contributor Author

Rizxcviii commented Nov 18, 2023

@Rizxcviii, glad you found that!
So the new behavior will be that if someone has an existing table, and they redeploy with a new name, that table will no longer be able to be updated?

Yes existing tables will no longer be able to be updated. I'm going to change it so existing tables cannot change their table names.

Any this does not apply to any other scenario? i.e. newly created table -> change table name?

No, new tables can change their table name as they please

@Rizxcviii
Copy link
Contributor Author

Rizxcviii commented Nov 20, 2023

@scanlonp

Screenshot 2023-11-20 143800.png

Screenshot 2023-11-20 144856.png

Screenshot 2023-11-20 144238.png

Screenshot 2023-11-20 144826.png

Screenshot 2023-11-20 144346.png

Screenshot 2023-11-20 144759.png

Screenshot 2023-11-20 145416.png

These were taken from a local deployment. This may be confusing, but new_test_table_v2 is actually the initial table. And that gets changed to old_test_table_v2

@Rizxcviii
Copy link
Contributor Author

@scanlonp could you take a look at this PR please? Thank you

Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

This looks great. I think all the code is good. From your screen shots, we are getting the behavior we want with replacing the prefix in the name.

Last request is adding a couple lines in the readme about this behavior. It will be quite annoying for a user to try to figure out what is happening to their table by looking up this pull instead of seeing a blurb in the docs.

After that we can get this merged!

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 6, 2023
@mergify mergify bot dismissed scanlonp’s stale review December 7, 2023 11:16

Pull request has been modified.

scanlonp
scanlonp previously approved these changes Dec 7, 2023
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

Thanks for getting this in!

Copy link
Contributor

mergify bot commented Dec 7, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed scanlonp’s stale review December 7, 2023 13:16

Pull request has been modified.

Copy link
Contributor

mergify bot commented Dec 7, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: eaa7285
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 7ac237b into aws:main Dec 7, 2023
9 checks passed
Copy link
Contributor

mergify bot commented Dec 7, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-redshift: table is dropped on table name change
4 participants