Adds "files add" http route. #235
Conversation
hackergrrl
force-pushed
the
files-http-add
branch
from
da296ed
to
e5a5f5d
Compare
|
This is pretty rad! Thank you @noffle |
| }) | ||
|
|
||
| fileAdder.on('end', () => { | ||
| if (written === 0 && file) { |
There was a problem hiding this comment.
is the file flag used to signal that at least one file was added?
There was a problem hiding this comment.
Yeah file is a boolean that is just used by L77 to generate a no data response from the server. There might be better ways of generating that response if no data is passed to the server in an add call.
There was a problem hiding this comment.
a 'no data' response? Like, being silent?
There was a problem hiding this comment.
if the file flag is true, but the written var hasn't been incremented, then a 500 server error is returned because the core failed even though the request had valid data parsed.
If at the end of parsing the request, if file is still false then the request was bad, so 400 Bad Request is returned
There was a problem hiding this comment.
so maybe 'failedToImport' flag?
There was a problem hiding this comment.
@diasdavid: sorry, can you clarify what you're asking to be named to failedToImport here?
There was a problem hiding this comment.
maybe file could be called didParse to flag that the parser emitted a file event, written makes sense to me for the data event from the importer when data has been stored, or maybe imported is better since it is currently storing the number of files imported
There was a problem hiding this comment.
I took a stab at making this a bit clearer. Lemme know what y'all think!
hackergrrl
force-pushed
the
files-http-add
branch
2 times, most recently
from
1bbb384
to
6728786
Compare
|
Getting test timeouts -- don't mind the reverts (WIP).. |
|
LGTM :) @noffle let me know if you have something to add (based on your last comment) or if I can merge it |
| @@ -12,7 +12,7 @@ const createTempNode = require('../utils/temp-node') | |||
| const repoPath = require('./index').repoPath | |||
|
|
|||
| describe('bitswap', function () { | |||
| this.timeout(20000) | |||
| this.timeout(80000) | |||
There was a problem hiding this comment.
Could we make this an env variable? Seems weird to keep defining it everywhere.
There was a problem hiding this comment.
These are often different depending on the test, that's why they are set specifically in here.
There was a problem hiding this comment.
why this unrelated change?
There was a problem hiding this comment.
I could break this into another PR, but timeouts were making my local tests fail. :(
There was a problem hiding this comment.
please make it at least a separate commit
There was a problem hiding this comment.
I think they already are -- see 5d55b3c. Did I miss one?
|
Might be good to add some failing tests, too - all of the ones here are positive. |
| fileAdder.on('data', (file) => { | ||
| serialize.write({ | ||
| Name: file.path, | ||
| Hash: bs58.encode(file.multihash).toString() |
There was a problem hiding this comment.
Please use mh.toB58String(file.multihash) from the multihashes package insteada
|
Assuming tests pass, should be good to go! |
|
@diasdavid teamcity's failure is a cached one it looks like; it's just whining about a timeout. If you're okay with how this PR looks, then please feel free to merge! |
|
@noffle looks good :) can we just get the commits squashed please? |
hackergrrl
force-pushed
the
files-http-add
branch
from
2a178f5
to
c5d8a94
Compare
|
woo add command! squashed commits and LGTM, gj @noffle |
|
@noffle one thing i forgot about this PR is that it doesn't address the cli looking for the daemon to be on. Also the CLI tests are not there that can now use the http resource for the tests. I have these tests as well I have written this code before and I just used it on your new add http-api resource and it works all ways! I can open up a separate PR once this is merged maybe 'add cli updates' https://gist.github.com/nginnever/39251f1d1572634e9699c875628dd00b |
|
@nginnever That's safe to put in another PR, right? It sounds like you have a lot of context for what needs to happen for these changes -- if you could make that happen that'd be super awesome. ❤️ |
|
Master needs to be rebased onto this branch for the clean merge. @noffle could you do that? @nginnever as @noffle suggested, please open a issue/PR with that missing part. |
hackergrrl
force-pushed
the
files-http-add
branch
from
c5d8a94
to
5326101
Compare
|
Only one more thing to go. This PR needs to update https://github.com/ipfs/js-ipfs/blob/master/src/cli/commands/files/add.js#L62, so that if the daemon is on, it doesn't fail. On that, it also needs cli tests to make sure add works when the daemon is on. |
|
can be closed in favor of PR |
|
ok, thank you for merging them and documenting what is missing, I really like it :) |
Tested to be compatible with a go-ipfs client requesting against the js-ipfs http api.