Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Support key/rm and key/rename #641

Merged
merged 4 commits into from
Dec 18, 2017
Merged

Support key/rm and key/rename #641

merged 4 commits into from
Dec 18, 2017

Conversation

richardschneider
Copy link
Contributor

Needed for ipfs/js-ipfs#1133

@daviddias
Copy link
Contributor

@richardschneider would you be so kind of standardizing the ipfs.key and ipfs.crypt APIs through https://github.com/ipfs/interface-ipfs-core while you are implementing it in both js-ipfs and js-ipfs-api?

@richardschneider
Copy link
Contributor Author

@diasdavid yes this is today's task.

@codecov
Copy link

codecov bot commented Dec 9, 2017

Codecov Report

Merging #641 into master will decrease coverage by 0.04%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
- Coverage   84.22%   84.18%   -0.05%     
==========================================
  Files         119      121       +2     
  Lines        1775     1783       +8     
==========================================
+ Hits         1495     1501       +6     
- Misses        280      282       +2
Impacted Files Coverage Δ
src/key/index.js 100% <ø> (ø) ⬆️
src/key/rm.js 75% <75%> (ø)
src/key/rename.js 75% <75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ef587a...e532bc9. Read the comment docs.

@daviddias
Copy link
Contributor

@richardschneider released interface-ipfs-core with new test, make sure to test with latest deps

@richardschneider
Copy link
Contributor Author

@diasdavid The added tests should be failing because js-ipfs does not yet support the Key API. The PR ipfs/js-ipfs#1133 needs to be merged. The PR relies upon ipfs/js-ipfs-repo#156 and an NPM release of js-libp2p-keychain.

Its the old chicken and egg problem.

@daviddias
Copy link
Contributor

The added tests should be failing because js-ipfs

@richardschneider this is the js-ipfs-api PR, not js-ipfs.

@richardschneider
Copy link
Contributor Author

richardschneider commented Dec 10, 2017

Yes, I now which repo I'm making changes to. js-ipfs-api runs the tests from interface-ipfs-core which in-turn need an"ipfs server" (either js-ipfs or go-ipfs).

@richardschneider
Copy link
Contributor Author

@diasdavid any comments?

@daviddias
Copy link
Contributor

@richardschneider can we get CI green?

@daviddias
Copy link
Contributor

@richardschneider one test is failing:

  1) .key
       .rm
         removes a key:

      Uncaught AssertionError: expected [] to have a length of 1 but got 0
      + expected - actual

      -0
      +1
      
      at ipfs.key.rm (node_modules/interface-ipfs-core/src/key.js:126:36)
      at f (node_modules/once/once.js:25:25)
      at streamToValue (src/utils/stream-to-json-value.js:4:300)
      at data (src/utils/stream-to-value.js:3:168)
      at ConcatStream.<anonymous> (node_modules/concat-stream/index.js:36:43)
      at finishMaybe (node_modules/readable-stream/lib/_stream_writable.js:607:14)
      at afterWrite (node_modules/readable-stream/lib/_stream_writable.js:470:3)
      at _combinedTickCallback (internal/process/next_tick.js:144:11)
      at process._tickCallback (internal/process/next_tick.js:180:9)

@richardschneider
Copy link
Contributor Author

@diasdavid Once ipfs-inactive/interface-js-ipfs-core#185 is merged. The above test will work.

@daviddias
Copy link
Contributor

@richardschneider watch out for the wording used in PR's.

The "fix" here:
image

Closed this PR by mistake.

@richardschneider
Copy link
Contributor Author

@diasdavid Please provide the wording you like.

@daviddias daviddias merged commit 113030a into master Dec 18, 2017
@daviddias daviddias deleted the keychain branch December 18, 2017 08:34
@ghost ghost removed the in progress label Dec 18, 2017
@daviddias
Copy link
Contributor

@richardschneider it doesn't matter if I like it or not, it is just how Github works. You say "fix X" and Github will close it if merged.

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