-
Notifications
You must be signed in to change notification settings - Fork 232
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
IPIP-280: App Conventions for HTTP Gateway Detection #280
base: main
Are you sure you want to change the base?
Conversation
This should help other application developers to have an idea of what's needed to add IPFS support to their application.
Ping. What's still required for this document to get merged? Don't confuse this with the document the add the |
Hi @markg85, two quick comments on this from some related discussion in ipfs/in-web-browsers#197:
|
ipfs/specs#280 License: MIT Signed-off-by: Marcin Rataj <[email protected]>
ipfs/specs#280 License: MIT Signed-off-by: Marcin Rataj <[email protected]>
* Using async-await * feat: support IPFS_GATEWAY env ipfs/specs#280 Signed-off-by: Marcin Rataj <[email protected]> Signed-off-by: Marcin Rataj <[email protected]> Co-authored-by: Marcin Rataj <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markg85 time to revisit this, now that we have IPIP process. some pointers below :)
I get the mindset here and it would help a lot! Not just with apps using IPFS but also with js libraries (think for example video.js) where we'd like to have IPFS integration in at some point in time too. Having a But.. how? When IPFS Companion is used, injecting a variable is doable. The downside is another point of metric collection to profile people. And for that reason alone i don't think this variable would be accepted. Even if we would all like it and add it to the spec, the browser security minded folks would probably have an issue with it. @lidel do you have an opinion on this?
Usually i'd say "yes, go for it!" but in this case, waiting is probably the smarter move. If you'd go for that you'd have a spec wip branch on top of my wip branch. That's a little too much wip. Wait till this spec lands (it's mostly there) before you consider making a spec on top of this one. |
Sweet! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick feedback:
- make this Kubo-agnostic
- document configuration location on MS Windows and macOS
- ensure manually added entries are not removed by starting/stopping local node
- allow for multiple local gateways to co-exist, and not "steal" traffic
integrations/GATEWAYS_FILE.md Reword the document to only handle the gateways file.
For Windows, it's not clear that the Roaming config ( |
Shouldn't the spec should say what text format is used? Is the filename in the file limited to ASCII, or otherwise is the file UTF-8, or in the encoding for the system locale, the system file encoding, etc? |
What happens if a gateway adds its URL to the global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markg85 I've rebased and split IPIP into spec and IPIP memo, integrated feedback from @dirkf and others, and turned this into a spec we would be sending to applications to simplify integrations like ones in ffmpeg, curl etc.
lmk if I've removed something that is still relevant, or if you feel we should include something new.
5. Try OS-specific paths | ||
- Windows | ||
1. `%LOCALAPPDATA%/ipfs/gateway` (local user) | ||
2. `%APPDATA%/ipfs/gateway` (roaming user) | ||
3. `%PROGRAMDATA%/ipfs/gateway` (global) | ||
- macOS | ||
1. `$HOME/Library/Application Support/ipfs/gateway` (user) | ||
2. `/Library/Application Support/ipfs/gateway` (global) | ||
- Linux | ||
1. `$HOME/.config/ipfs/gateway` (user) | ||
2. `/etc/ipfs/gateway` (global) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markg85 I'm not feeling strongly about having OS-specific paths or ~/.config
and /etc
: keep or remove?
afaik nothing implements these atm, but does not hurt to have them is spec to future-proof things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not feeling strong about those either.
They don't hurt or complicate things. The only reason i'd lean towards keeping it is because they clarify the windows part, mac/linux is effectively handled by points 1-4 as well.
So if it doesn't hurt, keep it. Else remove it.
When `gateway` file is present, the file contents MUST be interpreted as line | ||
separated (`\n` or `\r\n`) `text/plain` file. | ||
|
||
The first line of `gateway` file MUST be a valid `http://` or `https://` URL. | ||
|
||
Implementations that only need one URL SHOULD use the first one and ignore the rest of the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh ouch!
For this spec. I'd keep it at a first line requirement but not limited to 1 line. Aka, it's fine as-is.
Curl needs some attention to catch the multiline scenario regardless of this spec. As just any other data after the first line is gonna break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, my understanding is this problem was solved in curl/curl@859e88f#diff-1fa338b6a8d4b9a9ffd2243527ac4951c2576cb32dc28440f36f4f59e60643e2R785, the gateway
file can now have more than one line, curl ignores everything after the first one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
I'm leaving it up to your digression if you want to follow the curl 8.4.0 approach (meaning the spec needs to be reworded to 1 line and 1 only) or the curl 8.5.0 approach (first line must be the gateway, rest is ignored).
I would go for the 8.5.0 approach (aka, the spec as-written is ok). Given the other IPFS fixes in 8.5.0, that release is certainly the one to recommend for IPFS usage. Also, it's about to be released on Dec. 6th 2023.
This should help other application developers to have an idea of what's needed to add IPFS support to their application.
cc @Stebalien and @autonome Conversations with mainly you two have led to this spec being implemented as is written here in ffmpeg.