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

docs: add example of gateway backed by CAR file #147

Merged
merged 4 commits into from
Feb 2, 2023
Merged

docs: add example of gateway backed by CAR file #147

merged 4 commits into from
Feb 2, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jan 31, 2023

This PR adds:

Depends on #145.

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #147 (691d1b9) into main (6399b73) will increase coverage by 0.21%.
The diff coverage is 32.69%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
examples/gateway-car/api.go 31.37% <31.37%> (ø)
examples/gateway-car/main.go 35.18% <35.18%> (ø)

@hacdias hacdias changed the title wip: CAR gateway example example: add gateway backed by CAR Feb 1, 2023
@hacdias hacdias changed the title example: add gateway backed by CAR docs: add example of gateway backed by CAR Feb 1, 2023
@hacdias hacdias changed the title docs: add example of gateway backed by CAR docs: add example of gateway backed by CAR file Feb 1, 2023
@hacdias hacdias force-pushed the issue/141 branch 2 times, most recently from 4202ea2 to 973bd21 Compare February 1, 2023 11:31
@hacdias hacdias self-assigned this Feb 1, 2023
@hacdias
Copy link
Member Author

hacdias commented Feb 1, 2023

@lidel @aschmahmann I'm happy to receive reviews here. The code I mentioned in the comment above for LsUnixFsDir is gigantic and I feel like this is something that I don't want to ask users to have to write. Perhaps wrapping it around a helper could be beneficial?

@hacdias hacdias marked this pull request as ready for review February 1, 2023 11:40
@hacdias hacdias requested a review from lidel as a code owner February 1, 2023 11:40
Copy link
Contributor

@aschmahmann aschmahmann left a 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.

examples/gateway/car-file-gateway/api.go Outdated Show resolved Hide resolved
examples/gateway/car-file-gateway/api.go Outdated Show resolved Hide resolved
examples/gateway/car-file-gateway/api.go Outdated Show resolved Hide resolved
examples/gateway/car-file-gateway/api.go Outdated Show resolved Hide resolved
examples/gateway/car-file-gateway/api.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Feb 1, 2023

@aschmahmann thanks for the review. I have tackled the comments and massively simplified the example. I will work on adding some smoke tests.

(...) helper function to make this easier and better tested. (...)

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.

Copy link
Contributor

@aschmahmann aschmahmann left a 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.

@hacdias
Copy link
Member Author

hacdias commented Feb 1, 2023

@aschmahmann I'm just adding CI for the tests still :)

Base automatically changed from refactor/decouple-read-write-gateway to main February 2, 2023 01:04
Copy link
Member

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

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

examples/gateway-car/README.md Outdated Show resolved Hide resolved
examples/gateway-car/main.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Feb 2, 2023

@lidel I made the updates you requested and added a note to #149 so I don't forget to update this example once I move the DNSLink and subdomain code here. Regarding roots, I print all roots, whether there's 0 or 10. I will merge once CI is green.

@hacdias hacdias enabled auto-merge (squash) February 2, 2023 09:10
@hacdias hacdias disabled auto-merge February 2, 2023 09:12
@hacdias hacdias merged commit 302b279 into main Feb 2, 2023
@hacdias hacdias deleted the issue/141 branch February 2, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

gateway: create example that hosts a CAR file
3 participants