-
Notifications
You must be signed in to change notification settings - Fork 85
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
docs: add example of gateway backed by CAR file #147
Conversation
1e462b6
to
7c44868
Compare
Codecov Report
@@ Coverage Diff @@
## main #147 +/- ##
==========================================
+ Coverage 18.28% 18.50% +0.21%
==========================================
Files 94 96 +2
Lines 10181 10337 +156
==========================================
+ Hits 1862 1913 +51
- Misses 8058 8152 +94
- Partials 261 272 +11
|
4202ea2
to
973bd21
Compare
@lidel @aschmahmann I'm happy to receive reviews here. The code I mentioned in the comment above for |
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.
Left some comments on the complexity with Ls. At a high level though, this PR is a little difficult to merge with no tests since we have no idea if it's correct or not 🙃 (e.g. issues with go-ipfs-files, or directory listing).
You could add a couple smoke tests in a CAR for now and then move onto #142 which IIUC should be roughly testable using sharness tests. Hopefully the code from #142 and here should be almost entirely shared and could make its way into a helper function to make this easier and better tested.
Later as we do more refactoring we can attempt to make most of the complexity here go away with better types that are specific to what the gateway code currently needs.
@aschmahmann thanks for the review. I have tackled the comments and massively simplified the example. I will work on adding some smoke tests.
I'm almost entirely sure that the only difference between this PR and #142 will be the block service. It is quite trivial to have a Gateway API implementation given a block service that conforms to the interface. |
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.
Generally LGTM. We've got some basic tests and hopefully we'll get the main machinery better tested in #142. Approving to remove my prior "Request changes" since it looks good and because I might not be available later today to approve.
@aschmahmann I'm just adding CI for the tests still :) |
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.
lgtm, if you address small UX suggestions below, fine to merge
It may be useful to update this PR after #149 is done, and then expose subdomain on cid.ipfs.localhost:port
but also ok to merge as-is for now
This PR adds:
Depends on #145.