Skip to content
This repository was archived by the owner on Dec 20, 2024. It is now read-only.

bugfix: dfget overwrite /dev/null#1254

Closed
xuyixin1996 wants to merge 2 commits into
dragonflyoss:masterfrom
xuyixin1996:master
Closed

bugfix: dfget overwrite /dev/null#1254
xuyixin1996 wants to merge 2 commits into
dragonflyoss:masterfrom
xuyixin1996:master

Conversation

@xuyixin1996
Copy link
Copy Markdown

Ⅰ. Describe what this PR did

  1. bugfix: dfget will overwrite the /dev/null and caused some issue
  2. bugfix: supernode will panic when receive metrics report from dfget which the fileLength is -1 (default value)

Ⅱ. Does this pull request fix one issue?

N.A

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

N.A

Ⅳ. Describe how to verify it

dfget -u http://google.com -o /dev/null
ls /dev/null

before the patch, /dev/null will be a regular file
after the patch, /dev/null will not be changed

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/S labels Mar 19, 2020
@pouchrobot
Copy link
Copy Markdown
Contributor

We found this is your first time to contribute to Dragonfly, @xuyixin1996
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/dragonflyoss/Dragonfly/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 19, 2020

Codecov Report

Merging #1254 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1254      +/-   ##
==========================================
- Coverage   48.43%   48.41%   -0.02%     
==========================================
  Files         119      119              
  Lines        7573     7578       +5     
==========================================
+ Hits         3668     3669       +1     
- Misses       3608     3612       +4     
  Partials      297      297              
Impacted Files Coverage Δ
dfget/core/downloader/downloader.go 86.11% <0.00%> (-13.89%) ⬇️
supernode/daemon/mgr/scheduler/manager.go 22.60% <0.00%> (+0.68%) ⬆️

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 894eae8...0f5edf1. Read the comment docs.

res = append(res, err)
}

if err := m.validateFileLength(formats); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please do not edit this file directory, you should update swagger.yml first, and then auto-generate this file:
https://github.com/dragonflyoss/Dragonfly/blob/master/apis/README.md

yixin.xu added 2 commits March 19, 2020 16:04
Signed-off-by: yixin.xu <yixin.xu@shopee.com>
Signed-off-by: yixin.xu <yixin.xu@shopee.com>
Copy link
Copy Markdown
Member

@lowzj lowzj left a comment

Choose a reason for hiding this comment

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

I'm wondering in which scenario the flag --output should be set as /dev/null.
It may change the following codes to support writing /dev/null:

  • dfget/core#prepare: don't create target directory and temporary file
  • /dfget/core/downloader/p2p_downloader#TargetWriter: it should not be run in this case

@ML99bug
Copy link
Copy Markdown

ML99bug commented Mar 21, 2020

😄😄😄😄

@allencloud
Copy link
Copy Markdown
Contributor

@xuyixin1996 conflict happens, could you help to rebase to solve it? Thanks

@pouchrobot
Copy link
Copy Markdown
Contributor

ping @xuyixin1996
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

conflict/needs-rebase kind/bug This is bug report for project size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants