fix(redshift): UserTablePriviliges to track changes using table IDs #26955
Conversation
github-actions
bot
added
the
valued-contributor
There was a problem hiding this comment.
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.
|
Exemption Request: Already stated, an integration test already takes place internally with the internal interface. |
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@Rizxcviii sorry for the delay. I just saw these pings -- have been busy on other stuff past few weeks. I will review this tomorrow. |
There was a problem hiding this comment.
@Rizxcviii I'm fine with your code at this point. Just want to make sure that the tests fully explain what we're testing and then we're good to go
I also updated your PR description to describe the breaking change that i believe is happening here. Feel free to modify that comment how you see fit.
packages/@aws-cdk/aws-redshift-alpha/test/database-query-provider/privileges.test.ts
Show resolved
Hide resolved
aws-cdk-automation
removed
the
pr/needs-community-review
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
I agree with the breaking change, it would make sense to add it in here :) |
aws-cdk-automation
added
the
pr/needs-community-review
kaizencc
changed the title
There was a problem hiding this comment.
Thanks @Rizxcviii. Sorry for how long it took to get back to you again.
|
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
removed
the
pr/needs-community-review
|
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). |
|
@kaizencc for some reason mergify is giving the error
|
|
@mergify refresh |
✅ Pull request refreshed |
|
@mergify refresh |
✅ Pull request refreshed |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
To support solving #24246, there needs to first be a refactor of the UserTablePriviliges to instead record the table id. The reason being that new privileges would be granted/revoked on old tables that now have new names, since a change in table name caused an UPDATE action to be triggered. However the issue remains where, since the table has a new name, the grant/revoke action will be called on an invalid table name (i.e. the old table name).
We now use the table id to track tables, therefore preventing UPDATE events to be triggered.
This blocks #24308
This was originally PR #24874, however that had closed. @kaizencc requested changes, that I had added in here.
This was originally PR #26558, however that had closed. @kaizencc requested changes, that I have already implemented previously. However, he did not review them.
BREAKING CHANGE: the behavior of redshift tables has changed. UPDATE action will not be triggered on new table names and instead be triggered on table id changes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license