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(s3) compare unmarshaled policy #1758

Merged

Conversation

Tom1893
Copy link

@Tom1893 Tom1893 commented May 12, 2023

Description of your changes

In the observe policy in the bucket, the comparison of JSON strings was changed to the comparison between actual JSON objects to prevent the update loop. This way it recognizes that the two objects are the same.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

-->
Fixes #1757

I have:

  • Read and followed Crossplane's contribution process.
  • [ X] Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@haarchri haarchri requested a review from MisterMX May 12, 2023 14:21
@MisterMX
Copy link
Collaborator

@Tom1893 thank you so much for your contribution. Please add unit tests to verify that your changes are actually working and sign and squash your commits. Thanks in advance.

@kelvinwijaya
Copy link
Contributor

kelvinwijaya commented May 29, 2023

Hi @Tom1893,

Are we able to push this changes soon?

Copy link
Collaborator

@MisterMX MisterMX left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @Tom1893

Signed-off-by: Tom Dott (external expert on behalf of DB Netz AG) <tom.dott@deutschebahn.com>

fix(s3) add command to JSONNormalize

add unit test for policy

fix format

fix format

format
@Tom1893 Tom1893 force-pushed the fix-updateloop-in-bucketpolicy branch from f4eec87 to 36b4db9 Compare June 1, 2023 13:19
@MisterMX MisterMX merged commit 8282408 into crossplane-contrib:master Jun 1, 2023
8 checks passed
@kelvinwijaya
Copy link
Contributor

kelvinwijaya commented Jun 12, 2023

Hi @Tom1893 @MisterMX ,

I have just run test against the master build.

It seems error still persist and the Bucket Policy api are being triggered multiple times, state of the bucket also stuck in the "Creating" state.

image

Sample Bucket resource manifest is logged in #1771

@kelvinwijaya
Copy link
Contributor

Hey @Tom1893 @MisterMX ,

Based on my observations, if i put more than 1 statements in the policy, then the status will stuck in "Creating" state, else it will be just fine

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.

Fix S3 Bucket Policy update loop
3 participants