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

Extended 'Accidental Deletion Prevention' Verification #721

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

BruAlcaraz
Copy link
Contributor

I have created an extended deletion Prevention to avoid accidental deletion of Queues, Topics and subscriptions.
This new option is is enabled by default and can be disabled in the Options menu

@ErikMogensen
Copy link
Collaborator

How is this related to the existing setting?
image

@BruAlcaraz
Copy link
Contributor Author

If the Disable Accidental prevention is true then the new one is also going to be disabled automatically. But if the Disable Accidental prevention is false then you can disable just the extended part depending on user preference.

@BruAlcaraz
Copy link
Contributor Author

BruAlcaraz commented May 15, 2023

For the user, I made it visually clear by making the extended option true and disabling it when the original disable accidental option is also true.
image

image

Copy link
Collaborator

@ErikMogensen ErikMogensen left a comment

Choose a reason for hiding this comment

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

Thank you!

I think it will be confusing for some users what the different delete prevention alternatives will do. What do you think of combining them into one?

Also, can you provide more information about which scenario(s) this PR solves? Preferably in an issue that this PR can refer too.

Thanks!

…he existing 'Disable Accidental Deletition Prevention'
@BruAlcaraz BruAlcaraz force-pushed the main branch 2 times, most recently from bfc195a to 79069b6 Compare June 6, 2023 12:13
@BruAlcaraz
Copy link
Contributor Author

Hi @ErikMogensen,

I have merged the options as you suggested.

I don't know if there are any Issues reported as this was an issue that I encountered myself with some of my support engineers as they accidentally deleted the entire queue when trying to delete some dead-letter messages.

These changes extended the delete verification to anywhere where a main Item is been deleted (Queue, Topic, Subscription) to avoid accidental loss of possibly hundreds of queued messages.
The following 3 actions are the affected ones

image
image

@BruAlcaraz BruAlcaraz changed the title Added Extended Accidental Deletion Prevention Extended 'Accidental Deletion Prevention' Verification Jun 6, 2023
@BruAlcaraz
Copy link
Contributor Author

Hi @ErikMogensen what do you think now with the changes?

Copy link
Contributor Author

@BruAlcaraz BruAlcaraz left a comment

Choose a reason for hiding this comment

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

Changes done

@BruAlcaraz
Copy link
Contributor Author

@ErikMogensen, @paolosalvatori, @SeanFeldman
I haven't heard back from you guys in a while. Could you guys take a look at this PR?
I have had 2 of my support engineers deleting the Queue when trying to delete a selection of dead letters this is why I decided to suggest this change.

Thank you in advance.

Copy link
Collaborator

@ErikMogensen ErikMogensen left a comment

Choose a reason for hiding this comment

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

Great!

Sorry about the delay.

Just one thing. Please add curly brackets around the bodies belonging to the if statements, since that is the way it is usually done in this repo. Also, it may prevent someone from adding a deletion statement after such a body, not realizing it will be not be conditional.

@BruAlcaraz
Copy link
Contributor Author

@ErikMogensen Brackets added as requested.
Please let me know if it looks good now :)

Copy link
Collaborator

@ErikMogensen ErikMogensen left a comment

Choose a reason for hiding this comment

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

Thank you @BruAlcaraz.

We have revived #505 to set expected code style when making PRs.

@ErikMogensen ErikMogensen merged commit c74fdc0 into paolosalvatori:main Jul 26, 2023
2 checks passed
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.

3 participants