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

Add script for remove ledger #1650

Merged
merged 7 commits into from
Feb 16, 2021

Conversation

adenishchenko
Copy link
Contributor

Signed-off-by: anton.denishchenko anton.denishchenko@evernym.com

Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
@sovbot
Copy link

sovbot commented Feb 10, 2021

Can one of the admins verify this patch?

Copy link
Contributor

@askolesov askolesov left a comment

Choose a reason for hiding this comment

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

Looks good!

scripts/remove_ledger.py Outdated Show resolved Hide resolved
scripts/remove_ledger.py Outdated Show resolved Hide resolved
scripts/remove_ledger.py Outdated Show resolved Hide resolved
Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
@askolesov
Copy link
Contributor

(ci) test this please

Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

I'd highly recommend the script provide some feedback to the user and wait for confirmation.
It should clearly indicate it's about to delete a ledger, which ledger it's about to delete, describe the consequences, and then wait for confirmation from the user.

Some help text for the script would also be useful for those that are curious about what the script does, but don't want to read and fully understand the code.

@askolesov
Copy link
Contributor

@WadeBarnes Great ideas

Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

Looking good. A few minor suggestions.

scripts/remove_ledger.py Outdated Show resolved Hide resolved
scripts/remove_ledger.py Outdated Show resolved Hide resolved
Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
scripts/remove_ledger.py Outdated Show resolved Hide resolved
Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

DCO sign-off is missing from your last commit.

Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
@WadeBarnes
Copy link
Member

(ic) test this please.

Copy link
Member

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

Signed-off-by: anton.denishchenko <anton.denishchenko@evernym.com>
@askolesov
Copy link
Contributor

(ci) test this please

@askolesov
Copy link
Contributor

(ci) ok to test

@askolesov
Copy link
Contributor

@WadeBarnes It seems that all comments were processed. Could you review this again, please?

@WadeBarnes
Copy link
Member

@Toktar, @skhoroshavin, We just need approval from a code owner.

@Toktar Toktar merged commit 7ec31dc into hyperledger:master Feb 16, 2021
Toktar added a commit to Toktar/indy-node that referenced this pull request Mar 2, 2021
Add script for remove ledger

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: behalf
Toktar added a commit to Toktar/indy-node that referenced this pull request Mar 2, 2021
Add script for remove ledger

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: behalf

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable

Signed-off-by: toktar <renata.toktar@dsr-corporation.com>
Sign-off-executed-by: toktar <renata.toktar@evernym.com>
Approved-at: master-stable
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.

5 participants