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

Document IPNI HTTP provider specification #18

Merged
merged 4 commits into from
Aug 31, 2023
Merged

Conversation

masih
Copy link
Member

@masih masih commented May 23, 2023

Rendered

Note: I have opted for least disruptive spec relative to the current implementation. If you folks think this doesn't matter as much there are things that I'd like to change.

Fixes #16

Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

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

this looks like a good extraction of what we expect from the provider

IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
@masih masih force-pushed the masih/http_prov_spec branch from 7336659 to fa68cd2 Compare May 23, 2023 12:30
Copy link
Contributor

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

This is excellent

@rvagg
Copy link

rvagg commented May 23, 2023

It might be nice to leave open (and document here) the ability of announcement Addrs to include optional httpath specifier, and be clear how this interacts with the new path prefixes defined here. If I provide an httpath in a multiaddr, does it override /ipni/v1/ad/, or does it prepend to this?
My own limited use of this has involved wanting to change the default provider server that responds to /head and /{cid} to one that responds on /_ipni/head and /_ipni/{cid} because I want to reuse an existing server that has other things going on at the root. So I like that this is now namespaced; I also like optionality, but it's not critical now that this is so specific. i.e. custom httpath could be ruled out in this spec and I wouldn't be too upset, but I'd like the clarity.

@masih masih force-pushed the masih/http_prov_spec branch from fa68cd2 to 91abdef Compare May 24, 2023 10:08
@masih
Copy link
Member Author

masih commented May 24, 2023

@rvagg thank you for pointing httpath out. I have added a Namespacing section to spell out how I think the extra paths are to be handled here.

Can I ask you to give it a read and let me know what you think please?

Also captured ipni/go-libipni#42

@rvagg
Copy link

rvagg commented May 24, 2023

great! I've followed up in ipni/go-libipni#42

@masih masih marked this pull request as ready for review May 25, 2023 08:47
@masih
Copy link
Member Author

masih commented May 25, 2023

Cc @lidel @aschmahmann

@BigLep
Copy link

BigLep commented May 30, 2023

Doh - my original comment was in the wrong issue - my bad!

Copy link

@BigLep BigLep left a comment

Choose a reason for hiding this comment

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

Don't block on anything from me. Just left comments while reading through after it came up during the Content Routing WG on 2023-05-30. Thanks.

IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
Copy link

@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.

@masih added my usual comments about including explicit headers for HTTP Caching + some questions/concerns around /httpath

IPNI_HTTP_PROVIDER.md Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
IPNI_HTTP_PROVIDER.md Outdated Show resolved Hide resolved
@masih masih merged commit d62957f into main Aug 31, 2023
@masih masih deleted the masih/http_prov_spec branch August 31, 2023 08:02
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.

Formalize ipni advertisement publishing http spec
8 participants