bucket notifications - validate notifications on change (gh issue 8649) #8667
Conversation
alphaprinz
force-pushed
the
notif_gh_8649
branch
2 times, most recently
from
f48e291 to
e01196d
Compare
| @@ -275,7 +275,7 @@ async function delete_bucket(data, force) { | |||
| */ | |||
| async function bucket_management(action, user_input) { | |||
| const data = action === ACTIONS.LIST ? undefined : await fetch_bucket_data(action, user_input); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action, user_input); | |||
There was a problem hiding this comment.
Can we avoid adding a new argument user_input in the function signature?
There was a problem hiding this comment.
I didn't find a way.
The new input is required, the merged is not good enough.
When bucket name is changed, it is passed through a specific parameter ("new_name").
There was a problem hiding this comment.
But I don't see where you use the bucket name exactly.
Another option is maybe to check it later in the flow.
There was a problem hiding this comment.
Maybe you can add the notifications argument only? (I want to avoid confusion between data and user_input).
alphaprinz
force-pushed
the
notif_gh_8649
branch
from
e01196d to
72d34f9
Compare
alphaprinz
force-pushed
the
notif_gh_8649
branch
5 times, most recently
from
035ad66 to
1b2142c
Compare
| @@ -275,7 +275,7 @@ async function delete_bucket(data, force) { | |||
| */ | |||
| async function bucket_management(action, user_input) { | |||
| const data = action === ACTIONS.LIST ? undefined : await fetch_bucket_data(action, user_input); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action, user_input); | |||
There was a problem hiding this comment.
But I don't see where you use the bucket name exactly.
Another option is maybe to check it later in the flow.
alphaprinz
force-pushed
the
notif_gh_8649
branch
from
1b2142c to
c558e23
Compare
|
@alphaprinz could you add an automatic test for the fix? |
|
@dannyzaken can you PTAL ? |
…e 8649) Signed-off-by: Amit Prinz Setter <[email protected]>
alphaprinz
force-pushed
the
notif_gh_8649
branch
from
c558e23 to
d468e99
Compare
| @@ -275,7 +275,7 @@ async function delete_bucket(data, force) { | |||
| */ | |||
| async function bucket_management(action, user_input) { | |||
| const data = action === ACTIONS.LIST ? undefined : await fetch_bucket_data(action, user_input); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action); | |||
| await manage_nsfs_validations.validate_bucket_args(config_fs, data, action, user_input); | |||
There was a problem hiding this comment.
Maybe you can add the notifications argument only? (I want to avoid confusion between data and user_input).
| //if there's a change to the bucket's notifications, we need to test them | ||
| //if one of the specified notifications fail, we need to fail the user's request | ||
| if (user_input.notifications) { | ||
| const test_notif_err = await notifications_util.test_notifications(user_input.notifications, config_fs.connections_dir_path); | ||
| if (test_notif_err) { | ||
| throw_cli_error(ManageCLIError.InvalidArgument, "Failed to update notifications", test_notif_err); |
There was a problem hiding this comment.
Could you move this part to a separate function? (the function validate_bucket_args and then called it (the function has many lines before your change).
|
Re test - The test PR goes through this code path. |
Explain the changes
If one of them fails, the user's request is failed.
Issues: Fixed #xxx / Gap #xxx
Testing Instructions: