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

support add --nocopy <url> #6118

Closed
wants to merge 1 commit into from
Closed

support add --nocopy <url> #6118

wants to merge 1 commit into from

Conversation

jmank88
Copy link

@jmank88 jmank88 commented Mar 22, 2019

This PR enables add --nocopy support for URLs, by setting DagBuilderParams.URL when files.FileInfo.AbsPath() is a compatible URL. updating dependencies.

Draft until go-ipfs-cmds v0.0.4 is released
Fixes #6065

if absPath := fi.AbsPath(); filestore.IsURL(absPath) {
params.URL = absPath
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this given that we're setting AbsPath. We can probably get rid of this parameter all together and construct a WebFile in core/commands/urlstore.go instead.

Copy link
Author

Choose a reason for hiding this comment

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

Would that cause the data to be downloaded again?

Copy link
Member

Choose a reason for hiding this comment

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

In the urlstore command, we download the data on the server (I'm talking about two different commands, we can probably fix this later).

Copy link
Author

Choose a reason for hiding this comment

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

Ah I see - this code is completely redundant, and if we use a WebFile from urlstore, then the URL param is unused. Would urlstore itself be redundant also though? Is it still worth keeping around?

Copy link
Member

Choose a reason for hiding this comment

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

urlstore will be redundant but we should keep it around for compatibility (for a few releases as it is experimental).

@jmank88
Copy link
Author

jmank88 commented Mar 22, 2019

This approach has some side effects which urlstore does not (or at least doesn't log about) - all of the paths up to the root are added as directories:

> ipfs add --nocopy https://google.cloud.storage/v0/b/bucket/o/Qm...?alt=media&token=XXXXX
added Qm... https:/google.cloud.storage/v0/b/bucket/o/Qm...?alt=media
added Qm... https:/google.cloud.storage/v0/b/bucket/o
added Qm... https:/google.cloud.storage/v0/b/bucket
added Qm... https:/google.cloud.storage/v0/b
added Qm... https:/google.cloud.storage/v0
added Qm... https:/google.cloud.storage
added Qm... https:
> ipfs cat Qm...
hello world!

Also note the dropping of the &token... and the change from https:// to https:/ - I'm not sure what to make of that.

@Stebalien
Copy link
Member

The slash thing is the auto path canonicalization. Actually, I think we have to go back and fix this in go-ipfs-cmds. When we create the web file, we need to set fpath to something sane. Ideally the "filename" from the URL, if we can extract a reasonable one.

@jmank88
Copy link
Author

jmank88 commented Mar 22, 2019

I'll put up a fix for fpath. Then this PR is reduced to 'update the dependencies'.
Edit: ipfs/go-ipfs-cmds#158

License: MIT
Signed-off-by: Jordan Krage <jmank88@gmail.com>
@jmank88 jmank88 marked this pull request as ready for review March 22, 2019 17:21
@jmank88 jmank88 requested a review from Kubuxu as a code owner March 22, 2019 17:21
@Stebalien
Copy link
Member

This PR will probably be folded into #6019 once we fold #6120 into that as well (batch of interdependent changes)

@Stebalien
Copy link
Member

(folded)

@Stebalien Stebalien closed this Mar 22, 2019
@jmank88 jmank88 deleted the add-url branch March 23, 2019 14:41
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.

2 participants