Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat(issue-1852): now supports multiple api and gateways #1903

Merged
merged 5 commits into from
Mar 11, 2019
Merged

feat(issue-1852): now supports multiple api and gateways #1903

merged 5 commits into from
Mar 11, 2019

Conversation

grantlouisherman
Copy link
Contributor

@grantlouisherman grantlouisherman commented Feb 28, 2019 •
edited by alanshaw
Loading

resolves #1852

License: MIT
Signed-off-by: Grant Herman [email protected]

@grantlouisherman grantlouisherman mentioned this pull request Feb 28, 2019
Closed
@grantlouisherman grantlouisherman force-pushed the feat/multiple-api-gateways branch 5 times, most recently from 9ee14bb to 57bf1cf Compare March 2, 2019 06:29
@grantlouisherman
Copy link
Contributor Author

I think I need some guidance on how to get these tests passing. I have tried a bunch of different things and I am not sure what I need to do to get things to work.

@grantlouisherman grantlouisherman mentioned this pull request Mar 5, 2019
Closed
alanshaw
alanshaw suggested changes Mar 6, 2019
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Hey @grantlouisherman thanks for your contribution! Lots of minor feedback 🙏 but this PR is great! Do you think you could address the feedback and then if the tests are still failing I can help you to resolve the issues? 🚀 ⭐️

src/http/index.js Outdated
@@ -29,6 +29,24 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrsArr, createServerFunc, hapiInfoToMultiaddr, ipfs) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass hapiInfoToMultiaddr, it is in scope

src/http/index.js Outdated
@@ -29,6 +29,24 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrsArr, createServerFunc, hapiInfoToMultiaddr, ipfs) {
if (!serverAddrsArr.length) {
debug(Error('There are no addresses'))
Copy link
Member

Choose a reason for hiding this comment

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

Please just return [] here instead - it shouldn't be an error to not have any addresses to listen on.

src/http/index.js Outdated
@@ -29,6 +29,24 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrsArr, createServerFunc, hapiInfoToMultiaddr, ipfs) {
Copy link
Member

Choose a reason for hiding this comment

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

Please can we rename serverAddrsArr to just serverAddrs and then add a check as the first line of this function to convert it to an array if it is not already:

serverAddrs = Array.isArray(serverAddrs) ? serverAddrs : [serverAddrs]

src/http/index.js Outdated
@@ -29,6 +29,24 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrsArr, createServerFunc, hapiInfoToMultiaddr, ipfs) {
Copy link
Member

Choose a reason for hiding this comment

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

Please can we rename createServerFunc to createServer? By convention we don't add type information to our variable names in this project.

src/http/index.js Outdated
debug(Error('There are no addresses'))
}
// just in case the address is just string
let serversAddrs = [].concat(serverAddrsArr)
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed if we add the check as suggested above.

src/http/index.js Outdated
ipfs._print('Web UI available at %s', toUri(apiServer.info.ma) + '/webui')
this._gatewayServer = await Promise.resolve(
serverCreator.apply(this, [gatewayAddr, this._createGatewayServer, hapiInfoToMultiaddr, ipfs])
)
Copy link
Member

Choose a reason for hiding this comment

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

Same here, please rename to this._gatewayServers and just await the result:

this._gatewayServer = await serverCreator(gatewayAddrs, this._createGatewayServer, ipfs)

src/http/index.js Outdated
}

async stop () {
function stopServer (serverArr) {
Copy link
Member

Choose a reason for hiding this comment

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

Please rename stopServer -> stopServers and serverArr to servers.

src/http/index.js Outdated
function stopServer (serverArr) {
for (let i = 0; i < serverArr.length; i++) {
serverArr[i].stop()
}
Copy link
Member

Choose a reason for hiding this comment

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

You need to return a promise from this function:

function stopServers (servers) {
  return Promise.all(servers.map(server => server.stop()))
}

src/http/index.js Outdated
this._log('stopping')
await Promise.all([
this._apiServer && this._apiServer.stop(),
this._gatewayServer && this._gatewayServer.stop(),
this._apiServer && stopServer(this._apiServer),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this._apiServer && stopServer(this._apiServer),
stopServers(this._apiServers),

src/http/index.js Outdated
this._apiServer && this._apiServer.stop(),
this._gatewayServer && this._gatewayServer.stop(),
this._apiServer && stopServer(this._apiServer),
this._gatewayServer && stopServer(this._gatewayServer),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this._gatewayServer && stopServer(this._gatewayServer),
stopServers(this._gatewayServer),

@grantlouisherman
Copy link
Contributor Author

Hey @alanshaw I addressed your PR comments and left them in their own commit message. Once that looks good I can squash that commit.

alanshaw
alanshaw suggested changes Mar 7, 2019
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Looking great ❤️, all we need now is a test to check we can configure multiple API and Gateway servers.

src/http/index.js Outdated
@@ -29,6 +29,22 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrs, createServer, hapiInfoToMultiaddr, ipfs) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to pass hapiInfoToMultiaddr to this function - it’s in scope already

Copy link
Contributor Author

@grantlouisherman grantlouisherman Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hey when I don't pass that in and when I run jsipfs daemon it throws an error saying that it is not a function. Not 100% sure why, but I kept it in there for that reason. Maybe because it is being called within the scope of the start function that it does not have access to that function?

grantlouisherman
grantlouisherman commented Mar 8, 2019
View reviewed changes
test/cli/daemon.js Outdated
@@ -91,6 +91,33 @@ describe('daemon', () => {
}).catch(err => done(err))
})

skipOnWindows('should handle API Array and Gateway Array', function (done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @alanshaw I'm not sure if you wanted this to be tested in another location as well.

refactor: cleanup and add test
e57d0e9 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost assigned alanshaw Mar 11, 2019
@ghost ghost added the status/in-progress In progress label Mar 11, 2019
fix: guard against undefined _apiServers
1c38202 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw
Copy link
Member

@grantlouisherman hope you don't mind I added a test and cleaned up some of the code to make it a bit more robust to undefined values etc.

Tests are passing locally for me, I'll just wait for CI and then this'll be ready to merge. Thanks again! ⭐️

chore: increase timeout for windows
d5215c2 
License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@alanshaw alanshaw merged commit 4ad104d into ipfs:master Mar 11, 2019
@ghost ghost removed the status/in-progress In progress label Mar 11, 2019
@grantlouisherman
Copy link
Contributor Author

thanks @alanshaw for your help. I am going to try and pull another ticket if thats cool

@grantlouisherman grantlouisherman deleted the feat/multiple-api-gateways branch March 11, 2019 16:08
@alanshaw alanshaw mentioned this pull request Mar 11, 2019
Closed
50 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alanshaw alanshaw alanshaw approved these changes

Assignees

@alanshaw alanshaw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support exposing API/Gateway on multiple multiaddrs
2 participants
@grantlouisherman @alanshaw