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

move VersionOption after GatewayOption to fix #5422 #5424

Merged
merged 2 commits into from
Sep 6, 2018

Conversation

swedneck
Copy link
Contributor

@swedneck swedneck commented Sep 4, 2018

No description provided.

License: MIT
Signed-off-by: Tim Stahel <git@swedneck.xyz>
Copy link
Member

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Thanks!

Can you try adding a test case for that? (probably around https://github.com/ipfs/go-ipfs/blob/master/core/corehttp/gateway_test.go#L147-L177)

@swedneck
Copy link
Contributor Author

swedneck commented Sep 4, 2018

I'm not sure what you want me to add, i've never worked with the ipfs code (or go) before..

@Stebalien
Copy link
Member

I think this'll still miss, e.g., /api/v0/version. Is that correct? I think the correct order here is:

corehttp.MetricsCollectionOption("gateway"),
corehttp.IPNSHostnameOption(),
corehttp.GatewayOption(writable, "/ipfs", "/ipns"),
corehttp.VersionOption(),
corehttp.CheckVersionOption(),
corehttp.CommandsROOption(*cctx),

That is:

  1. Collect metrics first.
  2. Do any IPNS based redirects.
  3. Handle /ipfs, /ipns next.
  4. Handle /version.
  5. Check client's declared a go-ipfs version (if specified). Put this right before the read only commands option as we don't care about the client version for anything except the read-only commands.
  6. Forward to the read-only commands.

@Stebalien
Copy link
Member

Testing will be a bit tricky so we can handle that.

@swedneck
Copy link
Contributor Author

swedneck commented Sep 4, 2018

Should i change the order to that and push the commit here?

@Stebalien
Copy link
Member

@swedneck Yes (when you get a chance).

License: MIT
Signed-off-by: Tim Stahel <git@swedneck.xyz>
@Stebalien Stebalien merged commit bacb5b0 into ipfs:master Sep 6, 2018
@swedneck swedneck deleted the gateway-version-fix branch September 6, 2018 17:49
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.

3 participants