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

minor fixes to mc share download command; improved error messages and issue with --recursive option. (#2099) #2106

Merged
merged 1 commit into from
Apr 18, 2017

Conversation

badscooter23
Copy link
Contributor

@badscooter23 badscooter23 commented Apr 3, 2017

Verified that all the test cases below are working as expected...

mc ls play/foo should show two objects bar1, bar2

Scotts-MacBook-Pro-2:mc scott$ mc ls play/foo
[2017-04-03 13:10:00 PDT] 2.0MiB bar1
[2017-04-03 13:10:19 PDT] 1.1MiB bar2

All of these mc share download commands should generate an error ...

Scotts-MacBook-Pro-2:mc scott$ mc share download play/foo/invalid
mc: <ERROR> Object `play/foo/invalid` does not exist. 
Scotts-MacBook-Pro-2:mc scott$ mc share download play/foo/invalid --recursive
mc: <ERROR> Object `play/foo/invalid` does not exist. 
Scotts-MacBook-Pro-2:mc scott$ mc share download play/foo/bar
mc: <ERROR> Object `play/foo/bar` does not exist. 

Prior to this fix, the error message above was confusing, and the middle case failed without an error message.

mc share download play/foo/bar --recursive should generate two download share URLs...

Scotts-MacBook-Pro-2:mc scott$ mc share download play/foo/bar --recursive
URL: https://play.minio.io:9000/foo/bar1
Expire: 7 days 0 hours 0 minutes 0 seconds
Share: <.... URL deleted...>

URL: https://play.minio.io:9000/foo/bar2
Expire: 7 days 0 hours 0 minutes 0 seconds
Share: <.... URL deleted...>

Prior to this fix, this case would give an error instead of returning the two download share URLs.

@codecov-io
Copy link

codecov-io commented Apr 3, 2017

Codecov Report

Merging #2106 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2106      +/-   ##
=========================================
- Coverage    8.52%   8.51%   -0.01%     
=========================================
  Files          91      91              
  Lines        6806    6809       +3     
=========================================
  Hits          580     580              
- Misses       6091    6094       +3     
  Partials      135     135
Impacted Files Coverage Δ
cmd/share-download-main.go 0% <0%> (ø) ⬆️
cmd/client-errors.go 0% <0%> (ø) ⬆️

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 81f3ffa...f4bd165. Read the comment docs.

@harshavardhana
Copy link
Member

harshavardhana commented Apr 3, 2017

@badscooter23 Thanks for the patch, - can you have a more meaningful title changes for pull request #2099 really doesn't say much.

@badscooter23 badscooter23 changed the title changes for pull request #2099 minor fixes to mc share download command; (improved error messages and issue with --recursive option. (#2099) Apr 4, 2017
@badscooter23 badscooter23 changed the title minor fixes to mc share download command; (improved error messages and issue with --recursive option. (#2099) minor fixes to mc share download command; improved error messages and issue with --recursive option. (#2099) Apr 4, 2017
@vadmeste
Copy link
Member

vadmeste commented Apr 4, 2017

Can you also modify the commit title ? @badscooter23

@badscooter23 badscooter23 changed the title minor fixes to mc share download command; improved error messages and issue with --recursive option. (#2099) minor fixes to mc share download command; improved error messages and issue with --recursive option. (#2099) Apr 4, 2017
for _, url := range ctx.Args() {
_, _, err := url2Stat(url)
if err != nil {
fatalIf(errDummy(), "Object `"+url+"` does not exist.")
Copy link
Member

Choose a reason for hiding this comment

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

url2Stat can send different errors other than just object may not exist. Also doesn't use the traced err. So the origination of the error is lost

To fix the message we need to fix the the odd message is originated inside client-s3.go

for content := range clnt.List(isRecursive, isIncomplete, DirNone) {
objectsCh <- content
objectCount ++
Copy link
Member

Choose a reason for hiding this comment

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

The ++ here should be objectCount++

Copy link
Contributor

Choose a reason for hiding this comment

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

@badscooter23 - If you can amend your commit with this fix, we can take this forward.

@harshavardhana harshavardhana merged commit 9eb967c into minio:master Apr 18, 2017
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