core/commands/pin: add verbose pin #6403
Conversation
| Pins []string | ||
| Progress int `json:",omitempty"` | ||
| Pins []string | ||
| OngoingPins []string `json:",omitempty"` |
There was a problem hiding this comment.
I am not sure about this json tags.
There was a problem hiding this comment.
I'm actually not sure either. I'll look around.
| fmt.Fprintf(os.Stderr, | ||
| "Pinning %s\n", pin) | ||
| } | ||
| } else if showDigitProgress { |
There was a problem hiding this comment.
Should we also include the numeric progress while in verbose mode?
I'm kind of wondering whether there should really be separate verbose and progress modes. Maybe this work you're doing should expand the meaning of the progress flag instead of introducing a new one? cc @magik6k / @Stebalien do either of you have an opinion on whether or not we expand the progress flag to show more infomation? vs introducing a new flag for showing more verbose progress?
|
@reinerRubin mind getting the latest master merged into this work so |
reinerRubin
force-pushed
the
feat/cmds/pin-add-verbose-level
branch
from
64a3700 to
4f01773
Compare
|
@michaelavila, I have done a rebase, but circleci shows that golint is ok: |
|
@reinerRubin great! that's what I suspected, but because you didn't have the golint configs it wasn't running on CI |
test/sharness/t0085-pins.sh
Outdated
|
|
||
| test_expect_success "pin progress reported correctly" ' | ||
| cat test_pin_verbose_progress_err | ||
| grep -q "Pinning" test_pin_verbose_progress_err |
There was a problem hiding this comment.
Can we test for the whole line here? I think we have the hash available in $HASH, is that true?
| grep -q "Pinning" test_pin_verbose_progress_err | |
| grep -q "Pinning $HASH" test_pin_verbose_progress_err |
Another thing that would be nice to have is a test that shows progress for recursively pinning a hash that has a root block and other blocks underneath it. I think that test can be approached the same as the test you have here.
What do you think?
There was a problem hiding this comment.
Fixed with $HASH.
This test partially checks this. The command output is:
Pinning QmQZ1WVkddYPpRPm6vPpemhNacjwhupzSMzFEcYMisVpuY
Pinning QmQeYwtLpXuEmvG9vWfRhoeyNgXYw5KExozDTHyp3vBQzc
Pinning QmSijovevteoY63Uj1uC5b8pkpDU5Jgyk2dYBqz3sMJUPc
Pinning QmThbpxkSVyP4kjfrMfbqWJsnqkSTk1pTXj4dD8GD5L9H3
Pinning QmTbPEyrA1JyGUHFvmtx1FNZVzdBreMv8Hc8jV9sBRWhNA
But there are no explicit checks for these cids. Probably we can ask ipfs what cids to expect before pinning (idk).
|
@reinerRubin something else to consider: in the existing progress reporting implementation we just print to I'm curious, what are your thoughts on that? |
License: MIT Signed-off-by: Georgij Tolstov <[email protected]>
reinerRubin
force-pushed
the
feat/cmds/pin-add-verbose-level
branch
from
4f01773 to
b191604
Compare
|
@michaelavila, could you explain? I am not familiar with the ipfs logging system. Should pinned cids be written to the logs and If so, I think, it should be logged at the go-merkledag level with or without "ProgressTracker", because usually logs level is a global setting. |
Hello,
I have tried to add a verbose argument to the pinning command. It add output like this:
Currently this PR is more like a demo for the PR to go-merkledag [1].
@michaelavila
1 - ipfs/go-merkledag#38
#6042