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

feat: pin #107

Merged
merged 1 commit into from
Oct 22, 2017
Merged

feat: pin #107

merged 1 commit into from
Oct 22, 2017

Conversation

AdamStone
Copy link

@AdamStone AdamStone commented Apr 3, 2016

  • core
    • add
    • rm
    • ls
    • isPinned
    • isPinnedWithType
    • directKeys / recursiveKeys / internalKeys
    • flush (persist keys to datastore)
    • load (restore keys from datastore)
  • cli
    • add
    • rm
    • ls
  • http
    • add
    • rm
    • ls

Updated core interface to align with interface-ipfs-core, still need to update cli and http.

@jbenet jbenet added the backlog label Apr 3, 2016
@AdamStone AdamStone mentioned this pull request Apr 3, 2016
4 tasks
@daviddias daviddias removed the backlog label Apr 4, 2016
@daviddias daviddias changed the title Feature/pin feature/pin Apr 5, 2016
@daviddias
Copy link
Member

@AdamStone, js-ipfs has gone through a good amount of updates, including bug fixes and new features. Would you like to revisit pinning again? rebase master on your branch and continue the PR

@AdamStone
Copy link
Author

Sure, I guess I should have been rebasing rather than merging to begin with. I think the core implementation is basically done, so I'll update the description. The failing test should pass once this issue is fixed.

package.json Outdated
@@ -63,6 +63,7 @@
"bs58": "^3.0.0",
"debug": "^2.2.0",
"detect-node": "^2.0.3",
"fnv": "^0.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure about the licence for this module, can't find info on npm nor the repo.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good point. I'll swap in something with MIT license.

Copy link
Member

Choose a reason for hiding this comment

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

thanks :)

@daviddias
Copy link
Member

This is looking good @AdamStone :)

Would you also like to lead the ipfs-inactive/interface-js-ipfs-core#34, so that we have the same battery of tests for js-ipfs and js-ipfs-api for the pinning API.

Also, go-ipfs stores the root hash of the pinset inside level-db (datastore). Currently, js-ipfs doesn't use leveldb (datastore) because it is not implemented in ipfs-repo, would you like to tackle that as well -> ipfs/js-ipfs-repo#13

@AdamStone AdamStone force-pushed the feature/pin branch 2 times, most recently from 95384e9 to 9ab49ee Compare August 14, 2016 04:23
@AdamStone
Copy link
Author

@diasdavid Sorry for the delayed response, I see you've already started on ipfs/interface-ipfs-core#34 but I can continue with that if you like. Am I understanding correctly that it basically just involves migrating the core pinning tests from js-ipfs to interface-ipfs-core?

For the datastore, I did notice the discrepancy between js-ipfs and go-ipfs when working out how to store the pin set and ended up just putting it in the block store because I didn't see a good alternative. I just read through ipfs/js-ipfs-repo#13 but found it pretty confusing at first pass. I think I'd better do a little more research before committing to take that on.

@daviddias
Copy link
Member

@AdamStone sounds good. Let me know if you would like to go through it on IRC or a hangout, you can also join today's js-ipfs call https://github.com/ipfs/pm

@daviddias
Copy link
Member

@AdamStone good news, we've finished the Pin interface in interface-ipfs-core, so now in order to validate this Pin implementation, you just need to make sure it passes those tests

@AdamStone
Copy link
Author

Sounds good, I'll take a look tonight.

@AdamStone
Copy link
Author

AdamStone commented Aug 19, 2016

@diasdavid Can you clarify, when pin.add succeeds and returns "an array of objects that represent the files that were pinned", should this include indirectly-pinned objects when the add is recursive?

@daviddias
Copy link
Member

Since there is the type "recursive pin", we only need to know that the head was recursively pin. So no, we don't need to return them all.

@AdamStone
Copy link
Author

@diasdavid In that case, are there any circumstances where this should return more than just one object, since add expects a single hash? Or does that mean it should return the full pin set, not just the new addition?

@daviddias
Copy link
Member

The best is to see how go-ipfs does it through the CLI and match the expectations as closely as possible.

@AdamStone
Copy link
Author

AdamStone commented Aug 24, 2016

@diasdavid The go-ifps cli seems pretty flexible. You can pass one or more b58 hashes or ipfs paths like /ipfs/b58hash/link. For both add and rm it prints only the hashes that were changed by that operation, along with the type. I had tried to make the js pin CLI work with an ipfs path and behave like the go CLI, but I didn't notice you could pass more than one item. So I've adjusted the pin core methods to be more consistent with the CLI and to accept one hash/path or an array of them. Not sure if we want to test for those scenarios as well.

I found I needed to make a few modifications to the tests, so I opened a PR in interface-ipfs-core.

@loong
Copy link

loong commented Dec 2, 2016

Any updates on pinning?

@daviddias daviddias added status/deferred Conscious decision to pause or backlog and removed js-ipfs-backlog labels Dec 5, 2016
@AdamStone
Copy link
Author

I rebased what I have so far against current master here and in these related PRs for interface-ipfs-core and js-ifps-api. With these repos linked locally and with the changes in these PRs, all the pin tests are locally passing for me. However, there are placeholder workarounds in the code for two main blocking issues:

  1. The pin datastore key is /local/pins/js because using the key from go-ifps '/local/pins causes the tests to try to load pins from go-ipfs-repo, which doesn't work. Still unclear if the repo just needs to be migrated, since trying to do so causes version mismatch errors.
  2. The datastore is closed during the CLI core tests when trying to save and load pins due to some timing problem - the tests only pass if I manually open the datastore within the pin code.

@diasdavid I hate to leave this PR hanging again after getting this far, I was hoping this would be the final push to get all this wrapped up while I had the time to spend on it, but sorry to say I'm basically out of time now and I don't expect I'll be able to take this any further. Hopefully someone else can find it a useful starting point.

@daviddias daviddias added P0 Critical: Tackled by core team ASAP status/ready Ready to be worked and removed status/deferred Conscious decision to pause or backlog labels Oct 17, 2017
@daviddias daviddias changed the base branch from master to pin-api October 22, 2017 18:06
@daviddias daviddias merged commit df09eac into ipfs:pin-api Oct 22, 2017
@ghost ghost removed the status/ready Ready to be worked label Oct 22, 2017
@JonKrone JonKrone mentioned this pull request Mar 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P0 Critical: Tackled by core team ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants