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

feat: cli supports peer id path #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: cli supports peer id path #26

wants to merge 3 commits into from

Conversation

vasco-santos
Copy link
Member

For having easy and automatic deploys while keeping the same address in the server, this PR adds support to provide a path for a peer-id JSON file.

It can be tested as follows:

> node src/server/bin.js --peerId test/fixtures/peer-server.json

jacobheun
jacobheun approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

LGTM

Side note:
I just realized we are using camelcase options here and in webrtc star. It would probably be preferable to conform to more standard options, ie --peer-id, --disable-metrics, etc.

jacobheun
jacobheun reviewed Mar 26, 2020
View reviewed changes
src/server/bin.js
const peerData = fs.readFileSync(argv.peerId)
const peerId = await PeerId.createFromJSON(JSON.parse(peerData))
peerInfo = await PeerInfo.create(peerId)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, maybe we should add a log here when the peer id is not provided, just to warn that a new id will be created at each restart, and recommend using the peer id option.

@vasco-santos vasco-santos force-pushed the feat/cli-support-peer-id-path branch from 803b210 to d298ee9 Compare March 26, 2020 15:27
@vasco-santos
Copy link
Member Author

@jacobheun regarding the options I agree with you!

The easiest solution is to define alias in minimist to support both. Let me know your thoughts and I can PR both stardust and webrtc with that

@vasco-santos vasco-santos mentioned this pull request Mar 26, 2020
Open
@jacobheun
Copy link
Contributor

The easiest solution is to define alias in minimist to support both. Let me know your thoughts and I can PR both stardust and webrtc with that

Any reason to support both? We haven't released yet and it's already a breaking change. webrtc-star could have both supported there since it's released.

jacobheun
jacobheun reviewed Mar 26, 2020
View reviewed changes
src/server/bin.js Outdated Show resolved Hide resolved
@vasco-santos
Copy link
Member Author

I would support both to be consistent with webrtc-star and because this is the default of minimist. Otherwise, I need to use a different module

@jacobheun
Copy link
Contributor

and because this is the default of minimist.

ah, okay that's fine

@vasco-santos vasco-santos mentioned this pull request Mar 26, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jacobheun jacobheun jacobheun approved these changes

@mkg20001 mkg20001 Awaiting requested review from mkg20001

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

Successfully merging this pull request may close these issues.
2 participants
@vasco-santos @jacobheun