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

Add a --force flag to pin rm to ignore nonexistent pins and avoid bailing out #5716

Open
schomatis opened this issue Nov 2, 2018 · 6 comments
Labels
kind/enhancement A net-new feature or improvement to an existing feature topic/commands Topic commands

Comments

@schomatis
Copy link
Contributor

No description provided.

@schomatis schomatis added kind/enhancement A net-new feature or improvement to an existing feature topic/commands Topic commands labels Nov 2, 2018
@overbool
Copy link
Contributor

overbool commented Nov 2, 2018

@schomatis @kevina

In the original implementation, ipfs pin rm <invalid-hash> <valid-hash> <invalid-hash> will print Error: not pinned and stop unpinning the <valid-hash>. When we add --force option, Did you mean that we don't need to print Error: not pinned and continue to unpin <valid-hash>? If so, maybe we should modify the implementation about Unpin() in core/corerepo/pinning.go in the meantime. Because Unpin() will return error when it meet the first <invalid-hash>. Am I right?

@kevina
Copy link
Contributor

kevina commented Nov 2, 2018

I think unpin should be modified to continue to remove all valid pins. It should by default print an error on the invalid hash but continue anyway and then at the end print an error that not all pins can be removed at the end. If force is given it should ignore non-existent blocks but continue anyway and not return an error. In other words, the only think --force should do is not print or return an error.

ipfs block rm is a good model for this.

@schomatis
Copy link
Contributor Author

It should by default print an error on the invalid hash but continue anyway

Yes, exactly. I didn't express myself correctly, I meant that the -f flag should not error out on missing pins (it can print to stderr or to the log the the pins it didn't find).

@schomatis schomatis changed the title Add a --force flag to pin rm to ignore nonexistent pins and avoid Error: not pinned Add a --force flag to pin rm to ignore nonexistent pins and avoid bailing out Nov 2, 2018
@kevina
Copy link
Contributor

kevina commented Nov 2, 2018

@schomatis what I am saying is that pin rm should not bail out even if the --force flag is not given.

@Stebalien
Copy link
Member

So, forcing without a --force flag is a noticeable breaking change. How about:

  1. Collecting the errors as @overbool is doing in cmds/pin: use coreapi/pin #5843.
  2. If --force is not specified, emitting a final "errors occurred" error like we do in ipfs repo gc.

That way:

  1. We don't break anything.
  2. We still try to remove what we can.

We also need to involve the JS people. It looks like there was already a discussion here: https://github.com/ipfs/interface-ipfs-core/issues/127.

@Stebalien
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature topic/commands Topic commands
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants