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

BREAKING CHANGE: Add mc rb command to remove empty bucket #2662

Merged
merged 1 commit into from Feb 17, 2019

Conversation

poornas
Copy link
Contributor

@poornas poornas commented Jan 29, 2019

Fixes #2635 - Change mc rm --recursive behavior to
delete objects in bucket but not the
bucket itself.

@codecov
Copy link

codecov bot commented Jan 29, 2019

Codecov Report

Merging #2662 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2662      +/-   ##
=========================================
- Coverage    9.76%   9.69%   -0.07%     
=========================================
  Files         125     128       +3     
  Lines        9205   12106    +2901     
=========================================
+ Hits          899    1174     +275     
- Misses       8155   10782    +2627     
+ Partials      151     150       -1
Impacted Files Coverage Δ
cmd/main.go 0% <ø> (ø) ⬆️
cmd/client-s3.go 13.27% <0%> (+0.09%) ⬆️
cmd/rm-main.go 0% <0%> (ø) ⬆️
cmd/rb-main.go 0% <0%> (ø)
cmd/mirror-main.go 0% <0%> (ø) ⬆️
cmd/client-fs.go 29.46% <0%> (+0.82%) ⬆️
cmd/session.go 35.89% <0%> (-6.48%) ⬇️
pkg/hookreader/hookreader.go 19.23% <0%> (-5.77%) ⬇️
cmd/humanized-duration.go 59.57% <0%> (-4.53%) ⬇️
pkg/probe/probe.go 42.59% <0%> (-2.24%) ⬇️
... and 115 more

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 4b62f52...605432f. Read the comment docs.

@harshavardhana
Copy link
Member

We should mention this as a breaking change...

@@ -232,8 +232,8 @@ func (mj *mirrorJob) doRemove(sURLs URLs) URLs {
contentCh := make(chan *clientContent, 1)
contentCh <- &clientContent{URL: *newClientURL(sURLs.TargetContent.URL.Path)}
close(contentCh)

errorCh := clnt.Remove(false, contentCh)
isRemoveBucket := true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not remove the bucket in the target if the source has zero contents. This should be false.

image

Fine if it is left intentionally to work like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Praveenrajmani , changed it so mirror will not remove buckets

Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

Suggestion: This might be something apart from this PR (w.r.t remove log print discussions), I see the command mc rm --force minio/bucket do say that the object is being removed. But it wont actually remove without --recursive flag set. In such cases, we could perhaps fallback saying that they should use -r flag.

For ref,

image

@harshavardhana
Copy link
Member

The intention here is bucket is never removed from mc mirror or mc cp operation. Even with mc rm we have decided to not remove buckets anymore.

To remove buckets explicitly only command is mc rb

Copy link
Collaborator

@Praveenrajmani Praveenrajmani left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@vadmeste
Copy link
Member

vadmeste commented Feb 7, 2019

@poornas after introducing rb command, do you expect that rm is still able to remove objects from different buckets at the same time ?

@poornas
Copy link
Contributor Author

poornas commented Feb 7, 2019

@poornas after introducing rb command, do you expect that rm is still able to remove objects from different buckets at the same time ?

@vadmeste, yes - both the commands below will remove objects from different buckets, except that mc rb will delete bucket in addition.

➜  mc git:(rb) mc rb --force s3/tbucket11 s3/tbucket13
Bucket removed successfully `s3/tbucket11`.
Bucket removed successfully `s3/tbucket13`.

and

➜  mc git:(rb) mc rm --r --force s3/tbucket11 s3/tbucket13

@vadmeste
Copy link
Member

vadmeste commented Feb 7, 2019

@poornas I see, but I also mean this:
mc rm alias/

does it also need to traverse several buckets and remove all objects in them ?

@poornas
Copy link
Contributor Author

poornas commented Feb 7, 2019

@poornas I see, but I also mean this:
mc rm alias/

does it also need to traverse several buckets and remove all objects in them ?
@vadmeste , that functionality also remains the same as before.

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM except some minor changes

cmd/rb-main.go Outdated Show resolved Hide resolved
cmd/rb-main.go Show resolved Hide resolved
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM

Fixes minio#2635 - Change mc rm --recursive behavior to
delete objects in bucket but not the
bucket itself.
@kannappanr kannappanr merged commit e538599 into minio:master Feb 17, 2019
@deekoder deekoder changed the title Add mc rb command to remove empty bucket BREAKING CHANGE: Add mc rb command to remove empty bucket Feb 19, 2019
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.

None yet

5 participants