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.

Sign up for GitHub

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

Jump to bottom

cmds/pin: add force option for pin rm #5717

Closed
wants to merge 2 commits into from
Closed

cmds/pin: add force option for pin rm #5717

wants to merge 2 commits into from

Conversation

overbool
Copy link
Contributor

@overbool overbool commented Nov 2, 2018

Fixes: #5716

License: MIT
Signed-off-by: Overbool [email protected]

@overbool overbool requested a review from Kubuxu as a code owner November 2, 2018 02:04
@overbool
cmds/pin: add force option for pin rm
e6ea069 
License: MIT
Signed-off-by: Overbool <[email protected]>
@overbool overbool force-pushed the feat/cmds/pin/add-force-option branch from bebc8cf to e6ea069 Compare November 2, 2018 02:26
@kevina
Copy link
Contributor

kevina commented Nov 2, 2018

@overbool could you add a sharness test for this?

kevina
kevina reviewed Nov 2, 2018
View reviewed changes
test/sharness/t0085-pins.sh
@@ -56,6 +57,10 @@ test_pins() {
cat hashes | ipfs pin rm
'

test_expect_success "unpin non-existent hashes with force option" '
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure things are working right I would do

ipfs pin rm --force <invalid-hash> <valid-hash> <invalid-hash>

@overbool
cmds/pin: add testcase for force option
5c39fa5 
License: MIT
Signed-off-by: Overbool <[email protected]>
@overbool overbool force-pushed the feat/cmds/pin/add-force-option branch from cd99006 to 5c39fa5 Compare November 2, 2018 04:09
@Stebalien Stebalien requested a review from schomatis November 2, 2018 20:49
schomatis
schomatis reviewed Nov 2, 2018
View reviewed changes
Copy link
Contributor

@schomatis schomatis left a comment

Choose a reason for hiding this comment

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

We should report nonexisting hashes.

As suggested by @kevina I'm fine with making this the default behavior without the flag to mimic block rm .

@overbool
Copy link
Contributor Author

overbool commented Nov 8, 2018

@kevina @schomatis Currently, we had implemented the PinAPI.Rm in coreapi/pin.go, but we haven't used it yet. Should we use PinAPI.Rm?

Another question is should we deprecate corerepo/pinning.go? Because I think we can implement all function about pin in coreapi/pin.go.

@schomatis
Copy link
Contributor

I'm not sure, @magik6k might know more about that, but those issues seem more suited to be handled in another PR to avoid mixing different solutions.

@overbool
Copy link
Contributor Author

@magik6k Should we deprecate corerepo/pinning.go and implement all pin function in coreapi/pin.go.(like coreapi.BlockAPI)?

@Stebalien Stebalien requested a review from magik6k December 8, 2018 00:00
@magik6k
Copy link
Member

magik6k commented Dec 9, 2018

Sorry for the delay. Yep, we can remove corerepo/pinning.go and just use CoreAPI.

@overbool overbool mentioned this pull request Dec 13, 2018
Merged
@magik6k
Copy link
Member

magik6k commented Jan 7, 2019

I'm guessing we want #5843 in first

@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Jan 7, 2019
@guseggert guseggert closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevina kevina kevina left review comments

@schomatis schomatis schomatis left review comments

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

@magik6k magik6k Awaiting requested review from magik6k

Assignees
No one assigned
Labels
status/blocked Unable to be worked further until needs are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a --force flag to pin rm to ignore nonexistent pins and avoid bailing out
5 participants
@overbool @kevina @schomatis @magik6k @guseggert