Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[WIP] fix: add default datastore config #1461

Closed
wants to merge 2 commits into from

Conversation

vasco-santos
Copy link
Member

From my analysis, I concluded that our default repo datastore is equal to the Go one, but I would like to have an extra pair of eyes to check it.

This PR is part of another PR in js-ipfs-repo that aims to guarantee the interoperability of repos between js-ipfs and go-ipfs

It will be necessary to handle other configuration, what is not supported right now. Should I create an issue for tracking it? Or should this be a responsibility of js-ipfs-repo?

Fixes #1460

@ghost ghost assigned vasco-santos Jul 22, 2018
@ghost ghost added the status/in-progress In progress label Jul 22, 2018
@vasco-santos vasco-santos mentioned this pull request Jul 22, 2018
2 tasks
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

It would be great to see an example of how to customize this if the default repo is not being used. For example there is a custom repo example, https://github.com/ipfs/js-ipfs/tree/master/examples/custom-ipfs-repo, that uses fs exclusively (no level). If those configuration changes are made, I would expect the config to be set appropriately. If the two don't match, I would expect the node to fail to start.

This may require an update of https://github.com/ipfs/interface-datastore, and each of the datastores, to expose the info needed for the spec file.

@vasco-santos
Copy link
Member Author

+1 on having an example for this @jacobheun . I went deeper into the repo side for creating this set of PRs, but I am still confused about the possible configurations available. I think we have a clear lack of documentation and specs in the repo.

The problem with this PR is that we can specify the datastore in config, but it will end up with the same configuration anyway, as far as I understood. Correct?

@jacobheun
Copy link
Contributor

Yes, we definitely need to get more specs down for the repo, or we're at a higher risk of go and js diverging again in the future.

For configuration we really need to go one way. Currently as listed in that example it's all programatic, but as we implement the spec config we should probably transition to just using that. We'll need to support both to avoid breaking existing projects, but we should deprecate the current format.

Ideally we'd also have configurations for each of the datastores listed clearly in their individual repos so users can easily copy those into the mounts array. I think go has all of those in the go-ipfs repo itself but I think that makes adding new datastore support more cumbersome.

This PR will require an update to ipfs-repo, which you have started ipfs/js-ipfs-repo#173, to make sure that it fails if there is a mismatch in configuration. We'll want to make sure to add basic tests here as well, with more detailed tests in ipfs/interop.

@vasco-santos
Copy link
Member Author

Yes, we definitely need to get more specs down for the repo, or we're at a higher risk of go and js diverging again in the future.

For configuration we really need to go one way. Currently as listed in that example it's all programatic, but as we implement the spec config we should probably transition to just using that. We'll need to support both to avoid breaking existing projects, but we should deprecate the current format.

Ideally we'd also have configurations for each of the datastores listed clearly in their individual repos so users can easily copy those into the mounts array. I think go has all of those in the go-ipfs repo itself but I think that makes adding new datastore support more cumbersome.

Totally agree with everything you wrote above. That is my vision, as well.

This PR will require an update to ipfs-repo, which you have started ipfs/js-ipfs-repo#173, to make sure that it fails if there is a mismatch in configuration. We'll want to make sure to add basic tests here as well, with more detailed tests in ipfs/interop.

So, js-ipfs should send the provided config to js-ipfs-repo, and we should make sure that js-ipfs-repo fails if the spec provided is not compatible to what it knows. And here in js-ipfs, we should just a test with an incompatible spec, and guarantee that we receive the error. Correct?

@jacobheun
Copy link
Contributor

So, js-ipfs should send the provided config to js-ipfs-repo, and we should make sure that js-ipfs-repo fails if the spec provided is not compatible to what it knows. And here in js-ipfs, we should just a test with an incompatible spec, and guarantee that we receive the error. Correct?

Yes, and if the repo already exists and the spec is different it should probably fail as well. Yes, I think tests with a good and bad config would be sufficient here. More unit tests in js-ipfs-repo itself and the interop tests in interop should be sufficient.

@vasco-santos vasco-santos changed the title fix: add default datastore config (WIP) fix: add default datastore config Jul 26, 2018
@vasco-santos
Copy link
Member Author

After a productive call with @jacobheun we created an endeavour issue to tackle all the tasks regarding the repo interoperability. This issue aims to tackle the following set of tasks:

2) ipfs/js-ipfs

  • 1. Send the default/provided config (with datastore.spec)
  • 2. Present errors provided by js-ipfs-repo (have tests)
    • INIT: if the configuration is not compatible to what repo knows
    • START: if the repo is different from the spec

@alanshaw alanshaw changed the title (WIP) fix: add default datastore config [WIP] fix: add default datastore config Aug 7, 2018
@vasco-santos vasco-santos mentioned this pull request Aug 8, 2018
5 tasks
@vasco-santos vasco-santos force-pushed the fix/add-default-datastore-config branch 3 times, most recently from a63a631 to 07fd85f Compare August 8, 2018 13:00
@vasco-santos vasco-santos mentioned this pull request Aug 8, 2018
16 tasks
@vasco-santos vasco-santos force-pushed the fix/add-default-datastore-config branch from 07fd85f to 461e995 Compare August 8, 2018 13:49
@ghost ghost removed the status/in-progress In progress label Oct 20, 2018
@daviddias daviddias deleted the fix/add-default-datastore-config branch October 21, 2018 14:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants