Skip to content

bucket notifications - validate notifications on change (gh issue 8649)#8667

Merged
alphaprinz merged 1 commit intonoobaa:masterfrom
alphaprinz:notif_gh_8649
Jan 28, 2025
Merged

bucket notifications - validate notifications on change (gh issue 8649)#8667
alphaprinz merged 1 commit intonoobaa:masterfrom
alphaprinz:notif_gh_8649

Conversation

@alphaprinz
Copy link
Contributor

@alphaprinz alphaprinz commented Jan 9, 2025

Explain the changes

  1. As per aws spec, when bucket's notification change, all of the bucket's notification configuration are validated.
    If one of them fails, the user's request is failed.

Issues: Fixed #xxx / Gap #xxx

  1. put-bucket notification should check the connection before saving the information to the bucket #8649

Testing Instructions:

  1. Create a new bucket with a non-existing connect file or with a wrong connect info.
  2. Bucket creation should fail.
  • Doc added/updated
  • Tests added

@alphaprinz alphaprinz force-pushed the notif_gh_8649 branch 2 times, most recently from f48e291 to e01196d Compare January 9, 2025 07:16
shirady
shirady reviewed Jan 14, 2025
View reviewed changes
@alphaprinz alphaprinz requested a review from shirady January 14, 2025 22:00
@alphaprinz alphaprinz force-pushed the notif_gh_8649 branch 5 times, most recently from 035ad66 to 1b2142c Compare January 16, 2025 07:39
shirady
shirady reviewed Jan 16, 2025
View reviewed changes
@alphaprinz alphaprinz requested a review from shirady January 23, 2025 03:07
shirady
shirady reviewed Jan 23, 2025
View reviewed changes
@shirady
Copy link
Contributor

shirady commented Jan 23, 2025

@alphaprinz could you add an automatic test for the fix?
If I understand correctly you can use the file test_nc_nsfs_bucket_cli.test.js as the non-existing connection is while creating or updating a new bucket.

@nimrod-becker
Copy link
Contributor

nimrod-becker commented Jan 23, 2025

@dannyzaken can you PTAL ?

shirady
shirady reviewed Jan 26, 2025
View reviewed changes
@alphaprinz
Copy link
Contributor Author

alphaprinz commented Jan 27, 2025

Re test - The test PR goes through this code path.

@alphaprinz alphaprinz requested a review from shirady January 27, 2025 20:30
shirady
shirady approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

@shirady
Copy link
Contributor

shirady commented Jan 28, 2025

Re test - The test PR goes through this code path.

@alphaprinz Please feel free to mention this in the PR description.

@alphaprinz
bucket notifications - validate notifications conf on change (gh issu…
692ee36 
…e 8649)

Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
@alphaprinz alphaprinz merged commit a14183c into noobaa:master Jan 28, 2025
10 checks passed
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Jan 28, 2025
@alphaprinz
bucket notifications - validate notifications conf on change (gh issu…
d3a9b2d 
…e 8649) (noobaa#8667)

Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
This was referenced Jan 28, 2025
Merged
Merged
nimrod-becker pushed a commit to nimrod-becker/noobaa-core that referenced this pull request Aug 20, 2025
@alphaprinz @nimrod-becker
bucket notifications - validate notifications conf on change (gh issu…
7b0cd35 
…e 8649) (noobaa#8667)

Signed-off-by: Amit Prinz Setter <alphaprinz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shirady shirady shirady approved these changes

@romayalon romayalon Awaiting requested review from romayalon

@dannyzaken dannyzaken Awaiting requested review from dannyzaken

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alphaprinz @shirady @nimrod-becker