feat(repo): FSRepo #546
feat(repo): FSRepo #546
Conversation
btc
force-pushed
the
feat/repo-fsrepo
branch
2 times, most recently
from
211254a
to
ca4143d
Compare
| } | ||
| if err := r.Close(); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
if i'm reading this right, ipfs init -f would not blow out the datastore, correct?
I wonder which is easier on the user:
ipfs init -f # re-inits config
rm -rf ~/.go-ipfs/datastore
ipfs init -f # re-inits config, blows out everything
ipfs init --config # only reinits config
# no ipfs init -f
ipfs init --config # only reinits config
ipfs init --datastore # only reinits datastoreThere was a problem hiding this comment.
ipfs init -f # re-inits config
# "I used init -f. Why does it still say this key exists?"
# oh wait the datastore is still there. oops.
rm -rf ~/.go-ipfs/datastore
ipfs init -f # seems like the expected behavior
ipfs init --config # SGTM
# no ipfs init -f (i think init -f is nice to have as the default)
# both would probably be fine to provide
ipfs init --config
ipfs init --datastoreThere was a problem hiding this comment.
@jbenet that was a mistake. I'm thinking init -f should probably remove the datastore. Let me know what you think.
|
I'm in love with this PR. so much complexity reduction. ❤️ ❤️ ❤️ ❤️ ❤️ |
|
LGTM |
|
@jbenet Addressed the Since last review, I added a I'm satisfied with scope of this PR. Going to freeze at this point. Still TODO...
If this passes tests and still LGTU, ready for merge. |
|
|
||
| if u.FileExists(configFilename) && !force { | ||
| if fsrepo.IsInitialized(repoRoot) && !force { | ||
| return nil, errCannotInitConfigExists |
There was a problem hiding this comment.
maybe change error name to errAlreadyInited or errRepoExists? (nbd)
|
Comments:
Otherwise, LGTM, RTM. This is a great PR, thanks, and can't wait to have it merged. |
|
…empty datastore value no longer has bearing on node construction. Datastore setup now occurs in repo.
let me know if this looks off @whyrusleeping @jbenet
This may have been failing before.
@jbenet this removes everything under the path
The pkg.Interface style is modeled after heap.Interface. Generally, I find it helpful for interfaces that have many implementations. It provides clear distinction between the generic interface and the |n| implementations that implement it (which may be interface types themselves). For clients who cannot keep the repo name, one can imagine that the most likely rename is `ipfsrepo`. In that case, `ipfsrepo.Interface` remains meaningful. This is low-pri so it doesn't matter than much. But for the record, the repo.Interface feels appropriate in this use-case.
@jbenet @whyrusleeping
This introduces the
repo.Interfaceas described in #376.First step, bring the config under management (in-progress). Then, bring the datastore under management. Finally, make use the
repo.Interfacein thecore.IpfsNode.