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

fix: tag stdin with mtime and mode when piping to cli 'add' #2832

Merged

Conversation

kanej
Copy link
Member

@kanej kanej commented Mar 6, 2020

Converts the stdin stream to a async iterator of one file and tags that file with the mtime and
mode.

fixes #2763

@achingbrain
Copy link
Member

Neat! Could you add some tests around this please? The CLI handlers add take a getStdin function as a context argument so it should be relatively straightforward.

See the add from pipe test for an example.

@kanej
Copy link
Member Author

kanej commented Mar 9, 2020

Hi @achingbrain, I have added in a test to cover the piping with the mtime argument. The build failed shows a failure on the ipfs-http-client tests though the changes are in the cli package. Is this likely to be a gremlin in the test suite or an indicator of my capacity for truly epic bugs?

Comment on lines 246 to 249
: tagWithMetadata(getStdin(), {
mode: argv.mode,
mtime
}) // Pipe to ipfs.add tagging with mode and mtime
Copy link
Member

Choose a reason for hiding this comment

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

Would this work, without having to use normaliseAddInput?:

Suggested change
: tagWithMetadata(getStdin(), {
mode: argv.mode,
mtime
}) // Pipe to ipfs.add tagging with mode and mtime
: {
content: getStdin(),
mode: argv.mode,
mtime
} // Pipe to ipfs.add tagging with mode and mtime

Copy link
Member Author

Choose a reason for hiding this comment

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

Just gave it a go, but it seems ipfs.add is expecting an iterator:

TypeError: Cannot read property 'Symbol(Symbol.asyncIterator)' of undefined

We are able to get the job done if we turn the object into an async iterator

: (function * () {
   yield {
     content: getStdin(),
     mode: argv.mode,
     mtime
   }
 })()

Is there a iterator util for turning an element into a async iterator?

Copy link
Member

Choose a reason for hiding this comment

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

Weird, that should be supported. You should also be able to do:

: [{
     content: getStdin(),
     mode: argv.mode,
     mtime
   }]

Copy link
Member Author

Choose a reason for hiding this comment

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

That worked. Should I open a bug for the add function when passed:

{
   content: getStdin(),
   mode: argv.mode,
   mtime
}

Copy link
Member

Choose a reason for hiding this comment

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

I think this is because the test stubs ipfs.add to only return something when passed an iterable (async or otherwise) as the first argument:

ipfs.add.withArgs(matchIterable(), defaultAddArgs()).returns([{
  cid,
  path: 'readme'
}])

If you pass:

{
   content: getStdin(),
   mode: argv.mode,
   mtime
}

You're not passing an iterable so the stub doesn't match and undefined is returned from ipfs.add because JavaScript which causes the reported TypeError in the for await..of loop.

The test needs to be updated to this:

it('add from pipe with mtime=100', async () => {
  const cid = new CID('QmPZ9gcCEpqKTo6aq61g2nXGUhM4iCL3ewB6LDXZCtioEB')

  ipfs.add.withArgs(sinon.match({
    content: matchIterable(),
    mtime: { secs: 100 }
  }), defaultAddArgs()).returns([{
    cid,
    path: 'readme'
  }])

  const proc = cli('add --mtime=100', {
    ipfs,
    getStdin: function * () {
      yield Buffer.from('hello\n')
    }
  })

  const out = await proc
  expect(out).to.equal(`added ${cid} ${cid}\n`)

  const input = ipfs.add.getCall(0).args[0]
  expect(input).to.have.nested.property('mtime.secs', 100)
})

Copy link
Member

Choose a reason for hiding this comment

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

TBH it could even lose the last assertion (for the mtime.secs property) as it'll only fail if the ipfs.add stub doesn't match, which will cause the TypeError issue before it gets to that assertion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@achingbrain I have updated the usage and test as suggested. The mocking had to be changed in 'add from pipe' as well.

@kanej kanej force-pushed the fix/mtime-not-set-when-piping-from-stdin branch from ae912a8 to 5c21376 Compare March 17, 2020 10:54
@kanej kanej force-pushed the fix/mtime-not-set-when-piping-from-stdin branch from 5c21376 to 4a5c571 Compare March 24, 2020 13:26
@achingbrain
Copy link
Member

achingbrain commented Mar 24, 2020

Please could you rebase this on top of master or merge master into this branch to resolve the merge conflict? Then once CI is green it should be good to go.

Converts the stdin stream to a file object and tags it with the mtime and mode.

fixes ipfs#2763
@kanej kanej force-pushed the fix/mtime-not-set-when-piping-from-stdin branch from 4a5c571 to 43bf310 Compare March 24, 2020 16:36
@kanej
Copy link
Member Author

kanej commented Mar 24, 2020

@achingbrain I have rebased off of master. The build has failed on 'interop - electron-renderer', which I suspect is a build gremlin, can you kick off another build?

@achingbrain achingbrain merged commit 8c97de1 into ipfs:master Mar 25, 2020
mistakia pushed a commit to mistakia/js-ipfs that referenced this pull request Apr 3, 2020
Converts the stdin stream to a file object and tags it with the mtime and mode.

fixes ipfs#2763
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.

mtime and mode not set when piping from stdin
3 participants