Siderus/Orion
1
0
Fork 0
Code Issues Pull requests Projects Releases 15 Packages Wiki Activity Actions

Add check for already running daemon and custom ports for own daemon #92

Merged
kernelwhisperer merged 15 commits from feature/95-existing-ipfs-daemon into master 2018-04-29 13:29:20 -04:00
Conversation 9 Commits 15 Files changed 6 +264 -82
kernelwhisperer commented 2018-04-23 13:57:49 -04:00 (Migrated from github.com)
Copy link

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

## 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
koalalorenzo (Migrated from github.com) requested changes 2018-04-24 00:41:15 -04:00
app/daemon.js
@ -16,0 +84,4 @@
* Repo: '6',
* System: 'amd64/linux',
* Golang: 'go1.10'
* }
koalalorenzo (Migrated from github.com) commented 2018-04-24 00:40:01 -04:00
Copy link

We should probably move this into its own function to avoid repeating code.

We should probably move this into its own function to avoid repeating code.
app/daemon.js
koalalorenzo (Migrated from github.com) commented 2018-04-24 00:41:02 -04:00
Copy link

Shouldn't we change the default ports to the new ones only if the standard ones are taken?
What do you say?

Shouldn't we change the default ports to the new ones only if the standard ones are taken? What do you say?
kernelwhisperer commented 2018-04-26 12:27:58 -04:00 (Migrated from github.com)
Copy link

@koalalorenzo this is ready for review 😄

@koalalorenzo this is ready for review :smile:
👍 1
koalalorenzo (Migrated from github.com) requested changes 2018-04-27 02:13:55 -04:00
app/daemon.js
@ -7,4 +6,3 @@
import { app, dialog } from 'electron'
import pjson from '../package.json'
import { get as getAppRoot } from 'app-root-dir'
koalalorenzo (Migrated from github.com) commented 2018-04-27 02:08:42 -04:00
Copy link

Add some documentation to this method at least, as it will run cmd :D

Add some documentation to this method at least, as it will run cmd :D
app/daemon.js
@ -76,2 +154,3 @@
const ipfsProcess = spawnIPFSCommand('init')
// Prepare temporary file for logging:
const tmpLog = tmpFileSync({keep: true})
const tmpLog = tmpFileSync({ keep: true })
koalalorenzo (Migrated from github.com) commented 2018-04-27 02:10:10 -04:00
Copy link

Add few lines about ensureAddressesConfigured, why this should be called? what does it do?

Add few lines about `ensureAddressesConfigured`, why this should be called? what does it do?
app/daemon.js
@ -141,3 +217,3 @@
return exec(`${binaryPath} bootstrap add ${strMultiddr}`)
return executeIPFSCommand('bootstrap', 'add', `${strMultiddr}`)
}
koalalorenzo (Migrated from github.com) commented 2018-04-27 02:10:27 -04:00
Copy link

add docs here as well :)

add docs here as well :)
app/index.js
@ -34,0 +58,4 @@
if (apiVersion !== pjson.ipfsVersion) {
alertMessage += `\n\nPlease note: Orion was design with IPFS ${pjson.ipfsVersion} in mind, `
alertMessage += `while the available API is running ${apiVersion}.`
}
koalalorenzo (Migrated from github.com) commented 2018-04-27 02:11:09 -04:00
Copy link

Nice :) well done with this!

Nice :) well done with this!
koalalorenzo (Migrated from github.com) commented 2018-04-27 02:13:51 -04:00
Copy link

Why don't we move this in another function with a promise returning true/false based on the answer of the user?

Why don't we move this in another function with a promise returning true/false based on the answer of the user?
koalalorenzo commented 2018-04-28 17:21:30 -04:00 (Migrated from github.com)
Copy link

I made quite a few changes as now it works also on macOS:
https://github.com/Siderus/Orion/pull/92/files/8cd37a292e563af9078376fd046c9cef17e7474e..54b7481aca57ed023cd06f2699753148f352bc49

What changed:

  • Uses global variables defined only in index.js
  • Removes some methods not used anymore (due to the global variables)
  • Ensures that the UI is pointing to the right API endpoint if another instance is running
  • Uses exec instead of promised-exec (no longer maintained)
  • Removes unnecessary dependencies

@0x6431346e tell me what you think!

I made quite a few changes as now it works also on macOS: https://github.com/Siderus/Orion/pull/92/files/8cd37a292e563af9078376fd046c9cef17e7474e..54b7481aca57ed023cd06f2699753148f352bc49 What changed: - Uses global variables defined only in `index.js` - Removes some methods not used anymore (due to the global variables) - Ensures that the UI is pointing to the right API endpoint if another instance is running - Uses exec instead of `promised-exec` (no longer maintained) - Removes unnecessary dependencies @0x6431346e tell me what you think!
kernelwhisperer (Migrated from github.com) reviewed 2018-04-29 06:00:24 -04:00
app/daemon.js
@ -70,11 +151,9 @@ export function ensuresIPFSInitialised () {
if (isIPFSInitialised()) return Promise.resolve()
kernelwhisperer (Migrated from github.com) commented 2018-04-29 05:15:23 -04:00
Copy link

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?

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
kernelwhisperer (Migrated from github.com) commented 2018-04-29 06:00:18 -04:00
Copy link

I'm not sure if this will work, we need to test the app after building it.
EDIT: seems to be ok

I'm not sure if this will work, we need to test the app after building it. EDIT: seems to be ok
app/index.js
@ -56,20 +96,59 @@ app.on('ready', () => {
})
// Set up crash reports.
// Set up the needed stuff as the app launches.
kernelwhisperer (Migrated from github.com) commented 2018-04-29 05:12:47 -04:00
Copy link

ensureDaemonConfigured is already called on 136, do we need it again?

`ensureDaemonConfigured` is already called on `136`, do we need it again?
koalalorenzo (Migrated from github.com) reviewed 2018-04-29 07:54:11 -04:00
app/index.js
@ -56,20 +96,59 @@ app.on('ready', () => {
})
// Set up crash reports.
// Set up the needed stuff as the app launches.
koalalorenzo (Migrated from github.com) commented 2018-04-29 07:54:11 -04:00
Copy link

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.

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.
koalalorenzo (Migrated from github.com) reviewed 2018-04-29 09:51:11 -04:00
app/index.js
koalalorenzo (Migrated from github.com) commented 2018-04-29 09:51:11 -04:00
Copy link

You might be right! I think we need to re-introduce app-root-dir

You might be right! I think we need to re-introduce `app-root-dir`
kernelwhisperer commented 2018-04-29 11:23:47 -04:00 (Migrated from github.com)
Copy link

Looks ready to merge 👍

Looks ready to merge :+1:
koalalorenzo (Migrated from github.com) approved these changes 2018-04-29 13:29:13 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
koalalorenzo
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Siderus/Orion!92
No description provided.