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): UserTablePriviliges to track changes using table IDs' #26558

Closed
wants to merge 24 commits into from
Closed

fix(redshift): UserTablePriviliges to track changes using table IDs' #26558

wants to merge 24 commits into from

Conversation

Rizxcviii
Copy link
Contributor

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.


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

@Rizxcviii Rizxcviii changed the title Refactor/user table priviligies refactor(redshift): UserTablePriviliges to track changes using table IDs' Jul 28, 2023
@github-actions github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Jul 28, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team July 28, 2023 16:16
@github-actions github-actions bot added the p2 label Jul 28, 2023
@Rizxcviii
Copy link
Contributor Author

@kaizencc if you could review this please, I took consideration with your previous changes requested on the old PR

@Rizxcviii Rizxcviii changed the title refactor(redshift): UserTablePriviliges to track changes using table IDs' fix(redshift): UserTablePriviliges to track changes using table IDs' Jul 28, 2023
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.

@Rizxcviii
Copy link
Contributor Author

Exemption Request: According to @kaizencc this is a fix, rather than a refactor. However, what should be noted is that the integration test is running, just a change to an integ file is not necessary. The reason being that the change is an internal change with an internal interface, not accessible for public use.

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jul 31, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

hi @Rizxcviii! A few comments to get me to better understand the purpose of this PR. thanks for sticking with it!

@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 Aug 1, 2023
@Rizxcviii Rizxcviii requested a review from kaizencc August 2, 2023 14:55
@Rizxcviii
Copy link
Contributor Author

@kaizencc could you review this please?

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@Rizxcviii
Copy link
Contributor Author

Rizxcviii commented Aug 23, 2023

Bump

@kaizencc can you review this please.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 31, 2023
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2023

⚠️ The sha of the head commit of this PR conflicts with #26955. Mergify cannot evaluate rules on this PR. ⚠️

mergify bot pushed a commit that referenced this pull request Oct 6, 2023
…26955)

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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants