fix s3 notificationConfiguration #917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smcavallo
changed the title
|
@smcavallo can you squash commits for Clean history ? |
Signed-off-by: smcavallo <smcavallo@hotmail.com>
smcavallo
force-pushed
the
s3_notificationConfiguration
branch
from
05634a7
to
440013a
Compare
negz
approved these changes
Nov 19, 2021
| } | ||
| return true | ||
| }) | ||
| } |
There was a problem hiding this comment.
This is totally fine as is, but for future reference you could also pass in a couple ofcmpopts.SortSlices options, each with a less function for a slice of the relevant type of struct.
Comment on lines
+604
to
+606
| g := NewGomegaWithT(t) | ||
| actual, err := IsNotificationConfigurationUpToDate(tc.args.cr, tc.args.b) | ||
| g.Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
We try to avoid Gomega usage in our tests. I want to include this with the v0.21.1 release today so I plan to merge and quickly follow up to remove this.
|
Successfully created backport PR #962 for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: smcavallo smcavallo@hotmail.com
fixes s3 notificationConfiguration continually being updated.
Description of your changes
Updates
IsUpToDatecomparisons fornotificationConfiguration- most notably ignore theIdand ignore ordering when there are multiple configurations.Adds additional unit tests.
Note: this excludes the
Idproperties during the comparison. This should be fine because the actual rules are being compared.Per the docs https://docs.aws.amazon.com/cli/latest/reference/s3api/put-bucket-notification-configuration.html
Ignoring the
Idin the comparison is workaround since these properties are notLateInitializedFixes #896
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
IDin the console has not changedIDin the console has not changed