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

cluster/clean: prompt list of paths to be delete before processing (part 2) #993

Merged
merged 3 commits into from
Dec 16, 2020

Conversation

AstroProfundis
Copy link
Contributor

@AstroProfundis AstroProfundis commented Dec 15, 2020

What problem does this PR solve?

Prompt the user for file paths that are going to be delete and ask for confirmation, so that the user has a chance to cancel the process if there's anything unexpected.

This is a following up of #981 and closes #947

What is changed and how it works?

Calculate the file list before promoting for confirmation.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    图片

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Related changes

  • Need to cherry-pick to the release branch

Release notes:

cluster: refine `clean` sub command for better usability

@AstroProfundis AstroProfundis added type/enhancement Categorizes issue or PR as related to an enhancement. type/bug-fix Categorizes PR as a bug-fix category/usability Categorizes issue or PR as a usability enhancement. labels Dec 15, 2020
@AstroProfundis AstroProfundis self-assigned this Dec 15, 2020
@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 15, 2020
@AstroProfundis AstroProfundis changed the title cluster/clean: prompt list of paths to be delete before processing cluster/clean: prompt list of paths to be delete before processing (part 2) Dec 15, 2020
@@ -39,6 +44,44 @@ func (m *Manager) CleanCluster(name string, gOpt operator.Options, cleanOpt oper
return err
}

// calculate file paths to be deleted before the prompt
delFileMaps := make(map[string]set.StringSet)
Copy link
Member

Choose a reason for hiding this comment

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

delFileMap or delFilesMap

@lucklove
Copy link
Member

lucklove commented Dec 16, 2020

rest lgtm

@codecov-io
Copy link

codecov-io commented Dec 16, 2020

Codecov Report

Merging #993 (4842c6f) into master (efcaf79) will decrease coverage by 0.00%.
The diff coverage is 75.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #993      +/-   ##
==========================================
- Coverage   55.48%   55.47%   -0.01%     
==========================================
  Files         279      279              
  Lines       19709    19710       +1     
==========================================
- Hits        10936    10935       -1     
- Misses       7058     7062       +4     
+ Partials     1715     1713       -2     
Flag Coverage Δ
cluster 43.35% <75.86%> (+0.01%) ⬆️
dm 23.82% <0.00%> (-0.10%) ⬇️
integrate 49.74% <75.86%> (-0.01%) ⬇️
playground 20.28% <ø> (+0.01%) ⬆️
tiup 16.46% <ø> (ø)
unittest 23.06% <0.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cluster/manager/cleanup.go 49.18% <75.00%> (+15.84%) ⬆️
pkg/cluster/operation/destroy.go 56.61% <80.00%> (-2.26%) ⬇️
components/dm/command/scale_out.go 76.47% <0.00%> (-11.77%) ⬇️
pkg/cliutil/ssh.go 16.21% <0.00%> (-2.71%) ⬇️
pkg/cluster/api/dmapi.go 58.26% <0.00%> (-1.74%) ⬇️
pkg/cluster/api/pdapi.go 61.30% <0.00%> (+1.23%) ⬆️
pkg/utils/http_client.go 72.22% <0.00%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efcaf79...4842c6f. Read the comment docs.

@lucklove
Copy link
Member

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 16, 2020
@lucklove
Copy link
Member

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 16, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: 4842c6f

@ti-chi-bot ti-chi-bot merged commit 7668e42 into pingcap:master Dec 16, 2020
@AstroProfundis AstroProfundis deleted the refine-clean branch December 16, 2020 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/usability Categorizes issue or PR as a usability enhancement. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bug-fix Categorizes PR as a bug-fix type/enhancement Categorizes issue or PR as related to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure log_dir is not the same with data_dir for TiKV cluster
4 participants