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

pin: implement pin/ls with only CoreApi #6774

Merged
merged 2 commits into from
May 6, 2020

Conversation

MichaelMure
Copy link
Contributor

This PR use improvements from ipfs/interface-go-ipfs-core#49 and ipfs/interface-go-ipfs-core#50 to implement pin ls only using the CoreApi.

During this refactor I discovered a bug [I introduced in https://github.com//pull/6493, sorry] where the priority between pin type was not respected: when doing a ipfs pin ls --type=indirect, direct and recursive pins were not visited first so one of those could have been listed as indirect. This PR also fix this.

@MichaelMure
Copy link
Contributor Author

CC @Stebalien @magik6k

@Stebalien
Copy link
Member

#6705 fixes the issue you found.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Could you update to your interface-go-ipfs-core dep and rebase on master?

@MichaelMure
Copy link
Contributor Author

@Stebalien as I understand, there is two solutions here:

  1. Change PinAPI.Ls to return (chan coreiface.Pin, chan error) to match what Pin ls traverses all indirect pins #6705 did
  2. Keep the interface defined in feat: make the CoreAPI expose a streaming pin interface interface-go-ipfs-core#49 and add Pin.IsPinned(..) interface-go-ipfs-core#50 and return (<-chan Pin, error). In that case, rebasing this PR would essentially be copying my implementation over the one currently in master.

Can you confirm which one you'd like ?

@Stebalien
Copy link
Member

Let's go with the second. I think we need to evaluate using multiple channels for errors everywhere in the future but that's a larger change.

@MichaelMure
Copy link
Contributor Author

Should be good to go now

@MichaelMure
Copy link
Contributor Author

Someone to merge this one?

@Stebalien
Copy link
Member

I will look at this when I get a chance (probably not till after the holidays) but I'm going to need to think about the interface changes.

@MichaelMure
Copy link
Contributor Author

Rebased on master. It would be nice to merge that alongside ipfs/interface-go-ipfs-core#50.

@hsanjuan
Copy link
Contributor

hsanjuan commented Mar 5, 2020

I'll get back to review this after ipfs/interface-go-ipfs-core#50 is through.

@MichaelMure
Copy link
Contributor Author

@Stebalien do you think this could be included in the 0.5.0 ? That would be neat but it's fine if not.

@hsanjuan
Copy link
Contributor

hsanjuan commented Apr 3, 2020

hey @MichaelMure , this is not making it to 0.5.0 (currently very much in feature-freeze). That said these are on our radar for merging afterwards.

@Stebalien
Copy link
Member

Rebased on master.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Mostly LGTM but I have one question around cancellation.

for _, c := range keyList {
if keys.Visit(c) {
select {
case ch <- &pinInfo{
out <- &pinInfo{
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm missing something, we still need to select on the context, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it. Also, ctx should be the first parameter of pinLsAll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be good now.

@MichaelMure MichaelMure requested a review from Stebalien May 5, 2020 12:21
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thanks!

@Stebalien Stebalien merged commit 3339ce3 into ipfs:master May 6, 2020
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
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.

3 participants