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.

Sign up for GitHub

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

Jump to bottom

Require manual confirmation to purge database with purge-db #6154

Merged
merged 7 commits into from
Aug 20, 2024
Merged

Require manual confirmation to purge database with purge-db #6154

merged 7 commits into from
Aug 20, 2024

Conversation

macladson
Copy link
Member

@macladson macladson commented Jul 23, 2024 •
edited
Loading

Issue Addressed

#5028

Proposed Changes

The purge-db flag now requires manual confirmation in order to purge the database.
A new flag called purge-db-force has been added for those who still wish to purge with no intervention.

Additional Info

This is a pretty spicy breaking change as I suspect --purge-db is widely used.

See the alternative (although outdated) PR here which adds a purge-db-safe flag, which means the safety would effectively become opt-in. In my opinion this is unnatural, although more convenient as it doesn't require a breaking change.

@macladson macladson added ready-for-review The code is ready for review UX-and-logs backwards-incompat Backwards-incompatible API change work-in-progress PR is a work-in-progress and removed ready-for-review The code is ready for review labels Jul 23, 2024
@macladson macladson added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jul 28, 2024
macladson
macladson commented Jul 28, 2024
View reviewed changes
beacon_node/src/config.rs Outdated Show resolved Hide resolved
@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Aug 19, 2024
@macladson macladson added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Aug 19, 2024
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 20, 2024
@michaelsproul
Copy link
Member

@mergify queue

@mergify Mergify
Copy link

mergify bot commented Aug 20, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 32f2e05

mergify bot added a commit that referenced this pull request Aug 20, 2024
@mergify
Merge of #6154
834be3a
This was referenced Aug 20, 2024
Closed
Merged
@mergify mergify bot merged commit 32f2e05 into sigp:unstable Aug 20, 2024
29 checks passed
@macladson macladson deleted the purge-db-auto branch August 21, 2024 08:33
This was referenced Aug 21, 2024
Closed
Closed
AgeManning pushed a commit to AgeManning/lighthouse that referenced this pull request Sep 3, 2024
@macladson @AgeManning
Require manual confirmation to purge database with purge-db (sigp#6154
2a00cd7 
)

* Require manual confirmation to purge database

* Fix tests

* Rename to `purge-db-force and skip in non-interactive mode

* Do not skip when stdin_inputs is true

* Change prompt to be info logging to ensure consistent output

* Update warning text

* Move delete log after deletion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsproul michaelsproul michaelsproul approved these changes

Assignees
No one assigned
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. UX-and-logs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@macladson @michaelsproul