Skip to content

feat(cmds/add): --to-files option as files cp#8927

Merged
lidel merged 10 commits intomasterfrom
schomatis/feat/cmds/add/to-files
Sep 21, 2022
Merged

feat(cmds/add): --to-files option as files cp#8927
lidel merged 10 commits intomasterfrom
schomatis/feat/cmds/add/to-files

Conversation

@schomatis
Copy link
Copy Markdown
Contributor

@schomatis schomatis commented May 2, 2022

Closes #8504.

Sharness missing: #8504 (comment)

Example:

ipfs add p2p -r --to-files /

# added QmWSpN5PR7rkcnHQAWU8AvrAnqUtZnDfSzfFd9r5xnh7DJ p2p/listener.go
# added QmSj2YofPhB4i6LQwfBhdZZv5h7p1NNK8sTWYxT7xsKyUx p2p/local.go
# added QmaUuTcmN4y8G2E8i14U7VstZyrF6RrydbUanYkXt2hPXg p2p/p2p.go
# added QmTDkjKwN6ZxndDUnyLjdDF1yBxjr2TzBmmq2sDSgP82TE p2p/remote.go
# added QmQHnDfEdHDQPCksGNiY9asadBYMkmcQ5xKULog8FZLVJ3 p2p/stream.go
# added QmWw69M8XVQEV4tUQVYeUK9Er1v7M32X6Lz41Zg6ttzdnE p2p
 
ipfs files ls /p2p

# listener.go
# local.go
# p2p.go
# remote.go
# stream.go

@schomatis schomatis requested a review from lidel May 2, 2022 16:11
@schomatis schomatis self-assigned this May 2, 2022
@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch 2 times, most recently from e737b06 to d263d81 Compare May 2, 2022 20:25
Jorropo
Jorropo previously requested changes May 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Jorropo Jorropo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

SGTM thx, this looks good, however:

I can't get it working:

$ ./ipfs add test/ -r --pin=false --to-files /
Error: expected a file argument

Whatever I've tried, I get Error: expected a file argument.
My MFS is empty right now.

Also this could use some sharness tests.

@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented May 4, 2022

I can't reproduce your error. Let's just wait on @lidel's review on this one.

@lidel lidel added this to the go-ipfs 0.14 milestone May 16, 2022
lidel
lidel requested changes Jun 27, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@schomatis lgtm, I think if you rebase and add some basic sharness tests it will be ready to go.

(I've added some --help text around ipfs add --to-files and ipfs files write)

ps. fwiw I was also unable to reproduce @Jorropo's problem

@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch from 063556c to 6b45c92 Compare July 21, 2022 23:53
@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Jul 22, 2022

I'm now hitting @Jorropo's error Error: expected a file argument. Not sure if this is related to the rebase, but in any case I'll delve deeper into the add command.

@BigLep BigLep modified the milestones: kubo 0.14, kubo 0.15 Jul 22, 2022
@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Jul 25, 2022

This fails when running it through the daemon, fixing.

@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Jul 25, 2022 •
edited
Loading

It seems the iterator for multipartDirectory somehow consumes the state of the directory from where it's derived and we can't call it twice:

https://github.com/ipfs/go-ipfs-files/blob/88b4692b4642719bb8459a966f4b9a3da57c6e2d/multipartfile.go#L218-L221

(I could try to repackage what was iterated but seems the cleaner approach is to fix this upstream.)

@BigLep BigLep mentioned this pull request Jul 26, 2022
Closed
68 tasks
@schomatis schomatis mentioned this pull request Jul 26, 2022
Closed
@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Jul 26, 2022

Blocked on ipfs/go-ipfs-files#53.

@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Aug 2, 2022

We can't easily fix ipfs/go-ipfs-files#53 so working around it here.

@schomatis schomatis force-pushed the schomatis/feat/cmds/add/to-files branch from 7319e5b to d614316 Compare August 4, 2022 00:32
@schomatis schomatis requested a review from lidel August 4, 2022 17:31
@schomatis
Copy link
Copy Markdown
Contributor Author

schomatis commented Aug 4, 2022

@lidel Unified loops to use the iterator only once.

@lidel lidel removed this from the kubo 0.15 milestone Sep 1, 2022
@BigLep BigLep mentioned this pull request Sep 1, 2022
Closed
@BigLep BigLep requested a review from ajnavarro September 8, 2022 15:57
@ajnavarro ajnavarro requested a review from Jorropo September 9, 2022 10:30
@ajnavarro
Copy link
Copy Markdown
Member

ajnavarro commented Sep 9, 2022 •
edited
Loading

@Jorropo I tested the exact same command as you did and it is working fine. Can you have another look, please?
@lidel I see that there are some sharness tests. Do we need more of them?

@ajnavarro ajnavarro self-assigned this Sep 9, 2022
@BigLep
Copy link
Copy Markdown
Contributor

BigLep commented Sep 20, 2022

2022-09-20 conversation: @lidel will check to see if more sharness tests are needed. If there are more tests needed, @lidel will specify them and @ajnavarro will do them. If there is sufficient coverage @lidel will merge.

@lidel lidel mentioned this pull request Sep 20, 2022
Closed
@lidel lidel force-pushed the schomatis/feat/cmds/add/to-files branch from c37cee8 to ca2b36b Compare September 21, 2022 14:39
@lidel
tests(to-files): ensure error handling is covered
9bd0117 
this adds bunch of tests that guard UX around importing multiple files
into MFS, and the logic around trailing slash to indicate if the MFS
destination if a directory.

If the destination has a trailing slash, we ensure that the directory
exists and is a dir and not a file. this allows us to support
adding multipl files into MFS dir:

ipfs add file1.txt file2.txt --to-files /some/mfs/dir/
@lidel lidel force-pushed the schomatis/feat/cmds/add/to-files branch from ca2b36b to 9bd0117 Compare September 21, 2022 14:44
lidel
lidel approved these changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Works as expected – this is a real quality of life improvement.

  • Improved helptext (--help)
  • Added tests that guard UX around importing multiple files into MFS.
  • Improved logic around trailing slash to indicate if the MFS destination if a directory
    • If the destination has a trailing slash, we ensure that the directory exists and is a dir and not a file.
      This allows us to support adding multiple files into MFS, as long the destination is a dir and ends with slash: 
      $ ipfs add file1.txt file2.txt --to-files /at/some/mfs/dir/
...
$ ipfs files ls /at/some/mfs/dir/
file1.txt
file1.txt

@lidel lidel enabled auto-merge (squash) September 21, 2022 15:34
@lidel lidel dismissed Jorropo’s stale review September 21, 2022 16:08

"ipfs add test/ -r --pin=false --to-files /" is fixed now

@lidel lidel merged commit 9e5d0aa into master Sep 21, 2022
@lidel lidel deleted the schomatis/feat/cmds/add/to-files branch September 21, 2022 16:08
@kevincox kevincox mentioned this pull request Jan 15, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidel lidel lidel approved these changes

@ajnavarro ajnavarro Awaiting requested review from ajnavarro

@Jorropo Jorropo Awaiting requested review from Jorropo

Assignees

@ajnavarro ajnavarro

Labels

None yet

Projects

No open projects
IPFS Shipyard Team
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ipfs add --to-files=/path/in/mfs

5 participants

@schomatis @ajnavarro @BigLep @lidel @Jorropo