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

Extract provider module to go-ipfs-provider #6421

Merged
merged 4 commits into from
Jul 3, 2019
Merged

Extract provider module to go-ipfs-provider #6421

merged 4 commits into from
Jul 3, 2019

Conversation

michaelavila
Copy link
Contributor

@michaelavila michaelavila commented Jun 7, 2019 •
edited
Loading

@michaelavila michaelavila force-pushed the chore/extract-provider branch 2 times, most recently from 18e94bb to 561616f Compare June 7, 2019 17:37
@Kubuxu
Copy link
Member

Kubuxu commented Jun 7, 2019

The module itself depends on go-ipfs which isn't great. It will build but from a functional standpoint, it won't work well.

@michaelavila
Copy link
Contributor Author

@Kubuxu yes, I'm still working on this, thanks for the heads up though. There's a dependence on Pinner that I think is the only real issue.

@michaelavila michaelavila force-pushed the chore/extract-provider branch 2 times, most recently from 1df47b1 to 5aaa37a Compare June 10, 2019 19:02
michaelavila
michaelavila commented Jun 10, 2019
View reviewed changes
core/node/provider.go
@@ -115,3 +118,9 @@ func SimpleProviders(reprovideStrategy string, reprovideInterval string) fx.Opti
fx.Provide(SimpleReprovider(reproviderInterval)),
)
}

func pinnedProviderStrategy(onlyRoots bool) interface{} {
return func(pinner pin.Pinner, dag ipld.DAGService) simple.KeyChanFunc {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@magik6k @Kubuxu Any thoughts on a preferred alternative to this?

The reason for this is that the "old" reprovider depends on pin.Pinner which is a module within go-ipfs. To break that dependency I introduced a Pinner type in the go-ipfs-provider repository, but that is not the type of Pinner that's in the dependency injection container. To get around this I do the constructing of the dependency here, within go-ipfs, so that I can depend on pin.Pinner. The benefit, as I see it, is that this change is relatively small.

We could move the Pinner out as well into its own repository, but I really don't want to take on that large of a change right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to leave the NewPInnedProvider thing as a provider implementation within go-ipfs and extract the more generalistic ones (namely BlockstoreProvider). This looks very weird right now, and I don't need the PinnedProvider anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offline @magik6k said he's ok with this. Ultimately, he has some work in progress in #6387 that will change how all of this works for the better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hsanjuan I think that makes sense. Let me try it. And now that I think about it, ipfs-lite maybe doesn't have the concept of a Pinner, is that true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's true, no Pinner. Pinner is a go-ipfs specific concept (for now)

@michaelavila michaelavila force-pushed the chore/extract-provider branch 2 times, most recently from 3c77107 to 0d0ccd5 Compare June 10, 2019 20:09
@michaelavila michaelavila mentioned this pull request Jun 10, 2019
Closed
@michaelavila
Copy link
Contributor Author

michaelavila commented Jun 11, 2019 •
edited
Loading

@hsanjuan I have been looking at ipfs-lite trying to anticipate how you're going to integrate this code with it, and I have some questions:

  • the provider system needs to be Run() and Close(), how will that work in ipfs-lite?
  • Is ipfs-lite embedded in some other software?

You should be able to construct and test the provider behavior you need using the code here: https://github.com/ipfs/go-ipfs-provider. Can you try and see how that goes?

@michaelavila michaelavila marked this pull request as ready for review July 3, 2019 23:07
@Stebalien Stebalien self-assigned this Jul 3, 2019
@michaelavila
Copy link
Contributor Author

Currently depends on ipfs/go-ipfs-provider#11

@michaelavila michaelavila mentioned this pull request Jul 3, 2019
Merged
@Stebalien Stebalien merged commit 70e499a into master Jul 3, 2019
@Stebalien Stebalien deleted the chore/extract-provider branch July 3, 2019 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsanjuan hsanjuan hsanjuan left review comments

@Stebalien Stebalien Stebalien approved these changes

Assignees

@Stebalien Stebalien

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelavila @Kubuxu @Stebalien @hsanjuan