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.

Sign up for GitHub

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

Jump to bottom

feat: add $IPFS_PATH/gateway file #9156

Merged
merged 7 commits into from
Aug 8, 2022
Merged

feat: add $IPFS_PATH/gateway file #9156

merged 7 commits into from
Aug 8, 2022

Conversation

markg85
Copy link
Contributor

@markg85 markg85 commented Jul 31, 2022 •
edited by lidel
Loading

The file contains the gateway your node is hosting in the http://: format.
Structurally it works exactly the same as the API file.

IPIP: ipfs/specs#280

@markg85
feat: add gateway file
b2fadff 
The file contains the gateway your node is hosting in the http://<host>:<port> format.
Structurally it works exactly the same as the API file.
@markg85
Copy link
Contributor Author

markg85 commented Jul 31, 2022

cc @lidel, @Stebalien, @SgtPooki, @aschmahmann and @TheDiscordian
Just all those that were involved in one way or another with #8847

Note that @lidel offered to review this meaning it's just a "fyi" for the others.

I'd like to get this in the next kubo (0.15.0) as ffmpeg already relies on this and curl is probably soon going to follow suit.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 31, 2022

I'd like to get this in the next kubo (0.15.0) as ffmpeg already relies on this and curl is probably soon going to follow suit.

Our new release policy is a linux like one, where whatever is ready (merged into master and not absolutely crashing everything) is released, anything else is just bumped down.

We do that every 5 weeks (including a week of freeze / RC). The freeze start is 2022-08-18.

@markg85
Copy link
Contributor Author

markg85 commented Jul 31, 2022

Guess we have 2 1/2 week to get it "mergeable" then @Jorropo ;)

I have no clue why some tests fail btw.. Any hint there?

@Jorropo
Copy link
Contributor

Jorropo commented Jul 31, 2022

I've restarted the tests because the errors was weird, I do think it's related to your PR, we don't have THAT many flaky tests.

@Jorropo
Copy link
Contributor

Jorropo commented Jul 31, 2022 •
edited
Loading

@markg85 the linter is complaining, please run go fmt ./...

Jorropo
Jorropo requested changes Jul 31, 2022
View reviewed changes
repo/fsrepo/fsrepo.go Outdated Show resolved Hide resolved
cmd/ipfs/daemon.go Outdated Show resolved Hide resolved
Jorropo
Jorropo requested changes Aug 1, 2022
View reviewed changes
Copy link
Contributor

@Jorropo Jorropo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You need defers because if your program return early (the Errorf or Close fail) you never run your cleanup code.
defers also run if the goroutine panic.

repo/fsrepo/fsrepo.go Outdated Show resolved Hide resolved
Jorropo
Jorropo reviewed Aug 1, 2022
View reviewed changes
repo/fsrepo/fsrepo.go Outdated Show resolved Hide resolved
lidel
lidel previously requested changes Aug 2, 2022
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Needs green CI and a sharness test that confirms the file is created and includes URL matching the value in Addresses.Gateway.

@markg85
Copy link
Contributor Author

markg85 commented Aug 2, 2022

Green, yay :)
Don't accept yet though, I need to add a testcase for this.

@lidel lidel changed the title feat: add gateway file feat: add $IPFS_PATH/gateway file Aug 5, 2022
@markg85 markg85 requested review from lidel and Jorropo August 7, 2022 22:26
@markg85
Copy link
Contributor Author

markg85 commented Aug 7, 2022

Hi all,

I added a testcase in the file that already has a slew of gateway tests in there. Seemed like the logical place (either that or the daemon test file).

Is there anything else I need to do for this patch?

Jorropo
Jorropo approved these changes Aug 8, 2022
View reviewed changes
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

LGTM, thx for your patch 🎉

cmd/ipfs/daemon.go
@@ -806,6 +806,12 @@ func serveHTTPGateway(req *cmds.Request, cctx *oldcmds.Context) (<-chan error, e
return nil, fmt.Errorf("serveHTTPGateway: ConstructNode() failed: %s", err)
}

if len(listeners) > 0 {
if err := node.Repo.SetGatewayAddr(listeners[0].Addr()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems quite arbitrary to pick the index 0, I would maybe add a filter to pick localhost if possible but I don't think it's worth fixing.

@Jorropo Jorropo enabled auto-merge (squash) August 8, 2022 11:49
@Jorropo Jorropo removed the need_tests label Aug 8, 2022
@Jorropo Jorropo merged commit d419c0e into ipfs:master Aug 8, 2022
@NCGThompson NCGThompson mentioned this pull request Sep 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jorropo Jorropo Jorropo approved these changes

@lidel lidel Awaiting requested review from lidel lidel is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markg85 @Jorropo @lidel