Skip to content
This repository has been archived by the owner on Jun 21, 2020. It is now read-only.

Health check #258

Merged
merged 10 commits into from
Dec 1, 2019
Merged

Health check #258

merged 10 commits into from
Dec 1, 2019

Conversation

lenak25
Copy link
Contributor

@lenak25 lenak25 commented Nov 21, 2019

No description provided.

@lenak25 lenak25 added the not ready Not ready - do not merge yet label Nov 21, 2019
@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #258 into develop will increase coverage by 0.11%.
The diff coverage is 88.17%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #258      +/-   ##
===========================================
+ Coverage    75.67%   75.79%   +0.11%     
===========================================
  Files          103      105       +2     
  Lines         4991     5073      +82     
===========================================
+ Hits          3777     3845      +68     
- Misses        1214     1228      +14
Impacted Files Coverage Δ
src/worker/handlers/ProtocolHandler.js 60.67% <ø> (ø) ⬆️
src/main_controller/FacadeController.js 94.11% <ø> (ø) ⬆️
src/common/constants.js 100% <100%> (ø) ⬆️
.../actions/connectivity/BootstrapDiscoveredAction.js 100% <100%> (ø) ⬆️
src/ethereum/EthereumAPI.js 100% <100%> (ø) ⬆️
src/worker/EnigmaNode.js 67.54% <75%> (+0.28%) ⬆️
src/worker/controller/NodeController.js 78.88% <80.95%> (-1.75%) ⬇️
src/worker/handlers/WebServer.js 88.88% <88.88%> (ø)
src/worker/controller/actions/HealthCheckAction.js 92.3% <92.3%> (ø)
src/core/CoreRuntime.js 94.11% <0%> (-2.95%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324bdcf...d6071f3. Read the comment docs.

@lenak25 lenak25 removed the not ready Not ready - do not merge yet label Nov 26, 2019
@lenak25 lenak25 requested a review from assafmo November 27, 2019 09:24
Comment on lines 494 to 503
connectToBootstrap(peerInfo) {
connectToBootstrap(peerInfo, callback) {
this.dial(peerInfo, (connectionErr, connection) => {
if (connectionErr) {
this._logger.error(`Failed connecting to a bootstrap node= ${connectionErr}`);
return false;
} else {
this.notify(peerInfo);
return true;
}
callback(connectionErr);
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use promisify:

const { promisify } = require("util");
// ...
connectToBootstrap(peerInfo) {
  const dial = promisify(dial).bind(this);
  try {
    const connection = await dial(peerInfo); // Notice that connection is unused
  } catch (connectionErr) {
    this._logger.error(`Failed connecting to a bootstrap node= ${connectionErr}`);
    return false;
  }

  this.notify(peerInfo);
  return true;
}

Copy link
Member

Choose a reason for hiding this comment

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

Also Notice that dial can be implemented with promisify on dialProtocol and node.dialProtocol.
This way you won't have to do const dial = promisify(dial).bind(this);.


// core
try {
let regParams = await this.asyncGetRegistrationParams();
Copy link
Member

Choose a reason for hiding this comment

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

let -> const

// ethereum
if (this.hasEthereum()) {
try {
let eth = await this.ethereum().healthCheck();
Copy link
Member

Choose a reason for hiding this comment

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

let -> const

healthCheckResult.core.status = healthCheckResult.core.registrationParams.signKey != null;
if (this._controller.hasEthereum()) {
try {
let eth = await this._controller.ethereum().healthCheck();
Copy link
Member

Choose a reason for hiding this comment

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

const

Comment on lines 14 to 20
this._controller.engNode().connectToBootstrap(otherPeer, err => {
let success = false;
if (!err) {
success = true;
}
this._controller.logger().debug(`connection to bootstrap succeeded=${success}`);
});
Copy link
Member

Choose a reason for hiding this comment

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

Please use promisify with try/catch.

@assafmo
Copy link
Member

assafmo commented Nov 27, 2019

@lenak25 You can ignore the "let -> const" comments, but please use promisify, it will greatly improve code readability. 🙏

@lenak25
Copy link
Contributor Author

lenak25 commented Nov 27, 2019

@assafmo , code review comments were integrated.. please review again :)

assafmo
assafmo previously approved these changes Nov 28, 2019
@assafmo
Copy link
Member

assafmo commented Nov 28, 2019

@lenak25 Re: #262. Please add basic documentation (curl examples should suffice for now).

@assafmo
Copy link
Member

assafmo commented Nov 28, 2019

Also "Merging is blocked" due to "The base branch requires all commits to be signed." 🤷‍♂️

Try squashing this entire PR or resign every commit since branching out:
https://superuser.com/a/1123928/725752

git rebase --exec 'git commit --amend --no-edit -n -S' -i develop

@assafmo assafmo mentioned this pull request Nov 28, 2019
@lenak25 lenak25 merged commit 01009dd into develop Dec 1, 2019
@lenak25 lenak25 deleted the health_check branch December 1, 2019 12:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants