Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/corehttp: add support for posting directories to gateway #1543

Closed
wants to merge 1 commit into from
Closed

core/corehttp: add support for posting directories to gateway #1543

wants to merge 1 commit into from

Conversation

ebuchman
Copy link
Contributor

No description provided.

@jbenet jbenet added the backlog label Jul 30, 2015
License: MIT
Signed-off-by: Ethan Buchman <ethan@erisindustries.com>
@whyrusleeping
Copy link
Member

@ebuchman gitcop is really strict about the format, lol

@ebuchman
Copy link
Contributor Author

fixed! thanks gitcop for your fine services.

}

// add a directory recursively
func (i *gatewayHandler) addDirRecursive(file files.File) (*dag.Node, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/ipfs/go-ipfs/blob/master/core/commands/add.go#L329
...if common functions in corehttp and commands can be deduplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed this would be nice. I was hoping the addFile/addDir functions would have been exported from the core/commands, but they're not and are tied to that params struct, so it would have taken more reorganization. Also the functions in core/commands do more than we need for the gateway, like outputting progress bars, pinning, and ignoring hidden files (I've taken the opinion that if you send a tar ball, the whole thing will get added, hidden files included, and will not get pinned).

So, do you think we should add params for dealing with hidden files and pinning to the gateway?

Part of my hesitation in doing this (and in trying to use the functions in core/commands) was issues like #1322 which seem to suggest there is much greater reorg coming to the gateway to support the api. I figured deduplication would come then. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

For outputting progress bars, I think this is handled specifically by a PostRun function. So addDir can still be a common function.

That would be surprising to me if add in commands does pinning, but doesn't when through gateway.
Similarly for the case of hidden files.
In either choice, I think they should be consistent in both commands and corehttp.

Maybe deduplication can be done later (i.e. add TODO: after blocking commits have been taken care of), if the functionality in this PR is more pressing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, you're right about progress bars. the commands version tho does do some outputing (there's this outputDag function) but I suppose we can switch over that and not do it from the gateway. The thing about pinning though, is that add from the cli does it by default. I don't think the gateway should replicate that, since the gateway is a hosted service which is presumably being offered by some remote kind soul to benefit whichever local poor soul doesn't have an ipfs daemon running locally. It would seem to be to be bad manners to have that kind remote gateway pin everything that anyone adds, and hence keep it all locally. My understanding of the gateway is that it is meant to be a service offering a conduit to the ipfs network, not a fully featured endpoint.

as for the hidden files flag, happy to add that. wheres the proper place to put a param like that in a post request? do i restructure the body or is it fine to go in the url?

Copy link
Member

Choose a reason for hiding this comment

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

(I've taken the opinion that if you send a tar ball, the whole thing will get added, hidden files included, and will not get pinned).
So, do you think we should add params for dealing with hidden files and pinning to the gateway?

yeah, we've been thinking of adding a --pin option to both ipfs add and ipfs object put (defaulted true for add, false for object put).

maybe gateway can follow same convention as API (?<opt-key>=<opt-val>)

That would be surprising to me if add in commands does pinning, but doesn't when through gateway.
Similarly for the case of hidden files.

Agreed. add should default to the same in both cases.

It would seem to be to be bad manners to have that kind remote gateway pin everything that anyone adds, and hence keep it all locally.

Yep, agreed. maybe a config can disallow default gateway auto-pin. and result can be returned in a header or in the output.

My understanding of the gateway is that it is meant to be a service offering a conduit to the ipfs network, not a fully featured endpoint.

Indeed. though users also use their own http gateways for local apps. eventually we'll want some capability based auth --- i.e. add works if you have the right token.

@jbenet
Copy link
Member

jbenet commented Aug 2, 2015

(will CR soon)

is writable gateway broken atm, or did we fix that somewhere?

cc @cryptix

@cryptix
Copy link
Contributor

cryptix commented Aug 2, 2015

@jbenet nope, the putHandler is still broken on the writeable gateway (if nobody else fixed it) 😢

IIRC that handler was a nasty implementation of what ipfs object patch does, before that command existed. With that functionality it should be fairly simple to get it working.

@NodeGuy
Copy link

NodeGuy commented Aug 2, 2015

My two cents is that rather than shoehorning in an option (which will be awkward instead of using the entire POST body for the content) to disregard hidden files, it would be cleaner and waste less bandwidth not to send the hidden files to the gateway if the client doesn't want them added.

@ebuchman
Copy link
Contributor Author

ebuchman commented Aug 2, 2015

further support for that opinion is the simplicity with which one can archive a directory (client side) while ignoring the hiddens: http://nerdboys.com/2008/10/27/how-to-create-a-tar-file-that-excludes-hidden-files-and-folders/

@jbenet
Copy link
Member

jbenet commented Aug 5, 2015

the gateway is becoming complex. may be worth breaking it into it's own repo.

@whyrusleeping fix gx? I really want to start using it but every time i try, something goes horribly wrong. :(

@whyrusleeping
Copy link
Member

@jbenet i keep saying that i want ipns first, its not much fun without it

@rht
Copy link
Contributor

rht commented Aug 6, 2015

Yeah gateway shouldn't pin by default. Though I initially wasn't aware that
ipfs add also auto-pin until when I found ipfs repo gc doesn't remove
added files, if this is meant to be full features.

If gateway is to be put in separate repo, what about the common functions
with commands?

On Wed, Aug 5, 2015 at 9:01 PM, Jeromy Johnson notifications@github.com
wrote:

@jbenet https://github.com/jbenet i keep saying that i want ipns first,
its not much fun without it


Reply to this email directly or view it on GitHub
#1543 (comment).

.

@jbenet
Copy link
Member

jbenet commented Aug 6, 2015

@jbenet i keep saying that i want ipns first, its not much fun without it

why is ipns needed? all the hashes are put into the package.json

@whyrusleeping
Copy link
Member

@jbenet but then you have to use the hashes in your import paths.

@jbenet
Copy link
Member

jbenet commented Aug 6, 2015

@whyrusleeping

  • yes, and that's fine.
  • no, you can map a name to a hash in the local package.json

This discussion is unrelated to gx. let's have it somewhere else.

return
}

rootDir, err := unarchiveTarBall(tarBall)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why must every .tar.gzip archive be extracted? This makes the add and get not symmetric/reversible.

Copy link
Member

Choose a reason for hiding this comment

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

maybe the input is tarred by default, so an intentionally added tar archive might be inside another tar?

Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes the uploader knows that the transport format is must be tar.gz, even for a tar.gz file.

Copy link
Member

Choose a reason for hiding this comment

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

yeah :/ -- maybe if they're adding a file with our tools, it would be wrapped for them again. but good point. maybe should have a way to say "this is exactly what i want"

@rht rht mentioned this pull request Oct 16, 2015
5 tasks
@whyrusleeping
Copy link
Member

status here? I think we still want this. Although we can likely reopen as needed

@rht
Copy link
Contributor

rht commented Oct 18, 2015

This is superseded by #1845 (the remaining issue here is to write the test case: can't curl POST directories, only list of files).

@ghost ghost closed this Oct 19, 2015
@jbenet jbenet removed the backlog label Oct 19, 2015
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants