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

feat: restart command #152

Merged
merged 12 commits into from
Oct 19, 2020
Merged

feat: restart command #152

merged 12 commits into from
Oct 19, 2020

Conversation

strophy
Copy link
Contributor

@strophy strophy commented Oct 8, 2020

This PR implements the restart command described in #68

Issue being fixed or feature implemented

Now that the config command was implemented in #119, the config state is stored on disk. This allows us to restart a node with a single command, running updated code or config on restart.

What was done?

My approach here was just to combine the stop and start commands in a single new command. Oclif allows you to call commands programmatically, but I'm not sure if this is a good idea here, given we would just be passing the same arguments around anyway.

How Has This Been Tested?

  • Tested restarting nodes with different confs
  • Tested restarting after a config change was made to the running node

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

README.md Outdated Show resolved Hide resolved
src/listr/tasks/startNodeTaskFactory.js Outdated Show resolved Hide resolved
src/commands/restart.js Show resolved Hide resolved
README.md Show resolved Hide resolved

if (isMinerEnabled === true && config.get('network') !== NETWORKS.LOCAL) {
this.error(`'core.miner.enabled' option supposed to work only with local network. Your network is ${config.get('network')}`, { exit: true });
Copy link
Member

Choose a reason for hiding this comment

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

In the context of command (this), we should use this.error method. Everything was fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it because it wasn't working, did you test it? Here is my output on testnet with miner enabled:

# with this.error()
strophy@X250:~/Code/mn-bootstrap$ mn start
✖ this.error is not a function 

# with throw new Error()
strophy@X250:~/Code/mn-bootstrap$ mn start
✖ 'core.miner.enabled' option only works with local network. Your network is testnet. 

How do I find out what the current context is? It's impossible to console.log(this) because of listr overwriting any console output, and the debugger shows Error but not error as a function of this, so I still don't know what this actually refers to.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... my bad. This is the context of listr task too since it's in startNodeTask what we pass to Listr as a task :).

You can understand the context usually just by reading the code. If it's not clear you can run the command with debugger. In WebStorm it's easy. Just add a Node.JS run configuration with bin/mn as a start file and pass reset as application params.

@@ -58,7 +70,7 @@ function startNodeTaskFactory(dockerCompose) {

if (driveImageBuildPath || dapiImageBuildPath) {
if (config.get('network') === NETWORKS.TESTNET) {
this.error('You can\'t use drive-image-build-path and dapi-image-build-path options with testnet network', { exit: true });
throw new Error('You can\'t use drive-image-build-path and dapi-image-build-path options with testnet network', { exit: true });
Copy link
Member

Choose a reason for hiding this comment

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

Error constructor accepts filename as the second param. You don't need to pass anything except message (the first argument)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, is there a way to see the parameters required by a function in the IDE? I can't get this working in VS Code because using awilix seems to break the "Go to definition" function for all injected functions.

Copy link
Member

@shumkov shumkov Oct 19, 2020

Choose a reason for hiding this comment

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

Hm... it works in WebStorm. You can use something like "go to function" feature by the function name.

Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@shumkov shumkov merged commit 90f31e3 into dashevo:v0.16-dev Oct 19, 2020
@shumkov shumkov deleted the restart-command branch October 19, 2020 16:28
@shumkov shumkov mentioned this pull request Nov 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants