feat: match spec #5

hacdias · 2019-10-05T19:27:53Z

Matches spec on: https://github.com/ipfs-shipyard/cohosting
Should be released as 2.0.0.
Flag --no-pin removed since we do not create pins anymore.
Commands add, ls, rm, sync and gc added.
No command is alias to add for backwards compatibility.

Also, exports utility functions that can be imported somewhere else such as sync that might be used on IPFS Desktop.

Closes #1

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias · 2019-10-05T19:28:35Z

@olizilla I can't request for reviews since I don't have permission to edit this repository. Can you see #4 please?

/cc @lidel too!

hacdias · 2019-10-06T08:41:52Z

Also, I wonder if we should remove js-ipfs from this. I think ipfs-cohost should only connect to running daemons and not provide a daemon itself. It just increases the bundle of the tool that is not designed to be a daemon itself.

/cc @lidel @olizilla

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias · 2019-10-10T10:34:18Z

Let's block this until ipfs-shipyard/cohosting#6 is solved!

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias · 2019-10-20T10:02:33Z

@lidel @olizilla I would say this is unblocked. We already merged the spec for lazy and full cohosting and I just updated this PR.

hacdias · 2019-10-21T15:54:00Z

Thanks to @olizilla for passing this over to Shipyard!

lidel

Looks good (assuming small suggestions inline are addresed).

Ok to merge, but we should not release this to NPM until we address some things from #6 (comment), namely:

CLI: reusing existing HTTP API by default
API: deciding how to ensure small bundle (splitting CLI and API into separate packages?)

Anyway, discussion about those happens in #6

index.js

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

autonome

A couple of minor things noted in the comments, but otherwise looks generally fine.

Three areas I have questions about, required to take this tool to production level, but can take care of in later issues after this initial version merges:

Is there any input validation? I see lots of processing domain names but no checking the validity of them.
There isn't much in the way of error handling. Eg, what happens when network goes down/up, or operation returns error from IPFS for some reason - where do we catch it and how is it communicated to the user.
We need tests added, and written in a way that can be plugged into the automation matrix so can be run as regression check for future go/js IPFS releases (once we know what that way is).

README.md

hacdias · 2019-10-24T18:57:43Z

Is there any input validation? I see lots of processing domain names but no checking the validity of them.

Not much, I'm just letting ipfs handle it. But we can improve this later, for sure.

There isn't much in the way of error handling. Eg, what happens when network goes down/up, or operation returns error from IPFS for some reason - where do we catch it and how is it communicated to the user.

True, we're just printing it.

We need tests added, and written in a way that can be plugged into the automation matrix so can be run as regression check for future go/js IPFS releases (once we know what that way is).

For sure! Will create issues for this.

hacdias · 2019-10-24T18:59:44Z

Just waiting for the final call of @lidel on the prune command and then we can merge it!

hacdias · 2019-10-30T15:55:17Z

I'm merging this 😃 If there's anything need fixing, can be done on a different PR,

alanshaw · 2019-10-30T16:30:06Z

README.md


 ```console
-$ ipfs-cohost dist.ipfs.io tr.wikipedia-on-ipfs.org --no-pin
+$ ipfs-cohost ipfs.io --silent


Should be $ ipfs-cohost add ipfs.io --silent?

Can be both ways, but yes, will update!

alanshaw · 2019-10-30T16:34:25Z

README.md

-CumulativeSize: 6591536
+$ ipfs-cohost sync
+🔌 Using local ipfs daemon via http api
+✔  Snapshots synced!


Would be nice to know which sites were updated!

alanshaw · 2019-10-30T16:34:57Z

README.md

+```console
+$ ipfs-cohost prune [n]
+🔌 Using local ipfs daemon via http api
+✔  Cohosted websites cleaned!


Would be nice to know what was pruned and how much space was recovered.

Would be nice to know what was pruned

Can be added!

how much space was recovered

Unless someone calls ipfs repo gc, no space is, in fact, recovered, but that's not the job of ipfs-cohost. We should not repo gc something without user consent I believe

I agree, and actually if you're keeping many snapshots of the same data there's going to be a bunch of de-duping going on anyway making it really difficult to know how much space has been made available for gc anyway.

alanshaw · 2019-10-30T16:38:30Z

bin.js

-    --no-pin, Find the cumlative sizes but dont pin them
    --silent, -s  Just do your job
+    --full        Fully cohost a website
+    --lazy        Lazily cohost a website (default)


Why is lazy the default? Most websites aren't super big so personally I'd be ok with co-hosting them in full. Also, we want to encourage cohosting right? For people who don't take the time to read the help they'll just ipfs-cohost add wikipedia and bam they think they're hosting the whole thing when actually they're not.

We have an issue about that: ipfs-shipyard/cohosting#6, please take a look.

Personally, I wouldn't define the default for this in the spec. There's going to be different use cases and situations for lazy vs full. I think for a command line utility where I'm explicitly adding sites I want to cohost the default should be full, since actually lazily loading a site would require me to use a different application to explore it and start to lazily load bits of it. ipfs-cohost add wikipedia is effectively useless unless I then fire up my browser, load up my gateway, navigate to the site I just decided to cohost, and then start navigating around.

I totally get that in companion you'd want to set this to lazy by default, because it transparently redirects IPFS sites and it's likely to be more of an ambient setting than an explicit COHOST THIS NOW type of action.

cc @lidel

I think for a command line utility where I'm explicitly adding sites I want to cohost the default should be full [..]

I agree the CLI expectation is "full", but English Wikipedia is 650GB and we have no API to track progress of refs -r once add --full is executed :(
People may not understand why add takes days to complete.

Some ideas:

(A) default CLI to "full", but inform user about the size of added website, giving an option to cancel before fetch starts

(B) default CLI to "full", but fail if added website is more than X Megabytes and ask user to pass explicit --full to indicate they are ok with the cost

(C) return an error on add without explicit --lazy or --full (require explicit decision)

I think I like (B) the most, but perhaps there is a simpler way to address this?

Yes, I agree that --full may become the default but we should warn if it's bigger than X. So B for me.

B sounds great!

alanshaw · 2019-10-30T16:49:11Z

index.js

-      spinner.stop()
-      log(`📍 Pinned ${res.domain}`)
+  const newPath = `${path}/${getTimestamp()}`
+  await ipfs.files.cp([cid, newPath])


I'm not sure I understand how lazy works? If cid is the CID of a directory (which presumably it is for a website) then ipfs.files.cp will copy the directory and it's contents right? How does it know when I've visited pages on a website, when are they added to MFS?

@alanshaw if you don't have the contents on your repo, cp will just put a reference on MFS. Once you start navigating the website, you'll start fetching blocks and they'll be available.

If you cp something you don't have to MFS, it is not recursively fetched. That's why we call refs when the user wants to fully cohost something.

Gotcha - did not realise nodes were not recursively fetched.

N.b. you don't need to wrap the ipfs.files.cp args in an array, instead do await ipfs.files.cp(cid, newPath)

I've opened a PR against the docs to clarify this as I didn't realise it myself. ipfs-inactive/interface-js-ipfs-core#550

alanshaw · 2019-10-30T16:50:09Z

index.js

+    if (opts.fetchInBackground) {
+      ipfs.refs(cid, { recursive: true })
+    } else {
+      await ipfs.refs(cid, { recursive: true })


So this will cause your node to fetch all the contents...but it's not pinned and not in MFS and will be garbage collected.

It's cped to MFS just a little above your previous comment :P

feat: match spec

9111012

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias mentioned this pull request Oct 5, 2019

feat: support experimental cohosting ipfs/ipfs-desktop#1171

Closed

hacdias added 6 commits October 6, 2019 10:49

improve docs and clean output

88034a8

fix permissions and change name

188a6d9

docs: add more commands

ed20dd8

use dot instead of !

2d70906

docsss

ca337c0

Update README.md

34153da

hacdias mentioned this pull request Oct 6, 2019

ipfs files cp does not ensure recursive content ipfs-shipyard/cohosting#4

Closed

This comment has been minimized.

Sign in to view

fix: refs first

a26c892

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

This comment has been minimized.

Sign in to view

This was referenced Oct 7, 2019

ipfs-cohost - a cli to cohost sites on IPFS ipfs-shipyard/cohosting#1

Closed

Lazy cohosting ipfs-shipyard/cohosting#6

Open

feat: lazy/full cohosting

408b4d3

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias requested review from autonome and lidel October 21, 2019 15:53

Merge branch 'master' into feat/match-spec

d3ecda6

hacdias mentioned this pull request Oct 23, 2019

Should we bundle js-ipfs? #6

Closed

lidel approved these changes Oct 24, 2019

View reviewed changes

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

hacdias and others added 4 commits October 24, 2019 17:38

feat: fetch in the background

089758f

Co-Authored-By: Marcin Rataj <lidel@lidel.org>

rename gc to prune

4321443

simplify prune

c52b5d1

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

simplify even more

5a8b848

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

autonome approved these changes Oct 24, 2019

View reviewed changes

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

fix typos

7db25c1

This was referenced Oct 24, 2019

Testing #7

Open

Error Handling and Input Validation #8

Open

hacdias merged commit 8c11d19 into ipfs-shipyard:master Oct 30, 2019

hacdias deleted the feat/match-spec branch October 30, 2019 15:56

alanshaw reviewed Oct 30, 2019

View reviewed changes

hacdias mentioned this pull request Nov 3, 2019

Change to full by default #11

Closed

feat: match spec #5

feat: match spec #5

Uh oh!

Conversation

hacdias commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hacdias commented Oct 5, 2019

Uh oh!

hacdias commented Oct 6, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

hacdias commented Oct 10, 2019

Uh oh!

hacdias commented Oct 20, 2019

Uh oh!

hacdias commented Oct 21, 2019

Uh oh!

lidel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

autonome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hacdias commented Oct 24, 2019

Uh oh!

hacdias commented Oct 24, 2019

Uh oh!

hacdias commented Oct 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidel Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacdias Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanshaw Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

hacdias commented Oct 5, 2019 •

edited

Loading

lidel left a comment •

edited

Loading

lidel Oct 31, 2019 •

edited

Loading

hacdias Oct 30, 2019 •

edited

Loading

alanshaw Oct 31, 2019 •

edited

Loading