Add check for already running daemon and custom ports for own daemon #92
Conversation
kernelwhisperer
changed the title
kernelwhisperer
changed the title
app/daemon.js
Outdated
| IPFS_PATH: getIPFSRepoPath() | ||
| } | ||
| } | ||
| const ipfsProcess = spawn(binaryPath, ['daemon'], options) |
There was a problem hiding this comment.
We should probably move this into its own function to avoid repeating code.
app/daemon.js
Outdated
| const binaryPath = getPathIPFSBinary() | ||
| const ipfsRepoPath = `IPFS_PATH=${getIPFSRepoPath()}` | ||
|
|
||
| return exec(`${ipfsRepoPath} ${binaryPath} config Addresses.API /ip4/127.0.0.1/tcp/${ORION_API_PORT}`) |
There was a problem hiding this comment.
Shouldn't we change the default ports to the new ones only if the standard ones are taken?
What do you say?
kernelwhisperer
changed the title
|
@koalalorenzo this is ready for review 😄 |
app/daemon.js
Outdated
| binaryPath = path | ||
| } | ||
|
|
||
| export function executeIPFSCommand (command) { |
There was a problem hiding this comment.
Add some documentation to this method at least, as it will run cmd :D
app/daemon.js
Outdated
| @@ -102,23 +156,32 @@ export function ensuresIPFSInitialised () { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the multiAddr usable to connect to the local dameon via API | |||
| * 5001 -> 5101 | |||
There was a problem hiding this comment.
Add few lines about ensureAddressesConfigured, why this should be called? what does it do?
app/daemon.js
Outdated
| @@ -158,3 +219,22 @@ export function getSiderusPeers () { | |||
| return Promise.resolve(peers) | |||
| }) | |||
| } | |||
|
|
|||
| export function promiseRepoUnlocked (timeout = 30) { | |||
app/index.js
Outdated
| let alertMessage = 'An IPFS instance is already up!' | ||
| alertMessage += '\n\nWould you like Orion to connect to the available node, instead of using its own?' | ||
|
|
||
| if (apiVersion !== pjson.ipfsVersion) { |
There was a problem hiding this comment.
Nice :) well done with this!
app/index.js
Outdated
|
|
||
| getAPIVersion() | ||
| .then(apiVersion => { | ||
| let customPorts = false |
There was a problem hiding this comment.
Why don't we move this in another function with a promise returning true/false based on the answer of the user?
|
I made quite a few changes as now it works also on macOS: What changed:
@0x6431346e tell me what you think! |
app/index.js
Outdated
| }) | ||
| .then(promiseRepoUnlocked) // ensures that the api are ready | ||
| .then(() => ensureDaemonConfigured()) |
There was a problem hiding this comment.
ensureDaemonConfigured is already called on 136, do we need it again?
There was a problem hiding this comment.
One is for the daemon that starts, the other one is to ensure that the configuration is correct for the already running one, but for the configuration there is no live reload AFAIK.
app/daemon.js
Outdated
| @@ -126,7 +151,7 @@ export function ensuresIPFSInitialised () { | |||
| if (isIPFSInitialised()) return Promise.resolve() | |||
| console.log('Initialising IPFS repository...') | |||
| return new Promise((resolve, reject) => { | |||
| const ipfsProcess = spawnIPFSCommand('init') | |||
| const ipfsProcess = spawnIPFSCommand('init', `--api=${global.IPFS_MULTIADDR_API}`) | |||
There was a problem hiding this comment.
Why do we pass --api=${global.IPFS_MULTIADDR_API} here and when starting the daemon? since we call ensureDaemonConfigured beforehand, shouldn't it be set?
app/index.js
Outdated
| global.IPFS_PROCESS = null | ||
| global.IPFS_CLIENT = null | ||
|
|
||
| // Sets default values for IPFS configurations | ||
| global.IPFS_BINARY_PATH = 'go-ipfs/ipfs' |
There was a problem hiding this comment.
I'm not sure if this will work, we need to test the app after building it.
EDIT: seems to be ok
There was a problem hiding this comment.
You might be right! I think we need to re-introduce app-root-dir
|
Looks ready to merge 👍 |
What changed?
This PR allows orion to connect to an existing IPFS Api (if available at
http://localhost:5001)https://dev.siderus.team/issues/95
https://github.com/Siderus/Orion/issues/89