Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrections and enhancements to the Healthcheck endpoints #2313

Merged
merged 8 commits into from
Dec 12, 2019

Conversation

mcbhenwood
Copy link
Contributor

@mcbhenwood mcbhenwood commented Dec 9, 2019

Description of change

This PR addresses a few things:

  1. Pingdom needs to be able to call two healthcheck points per service. One for critical failures (P1); the other for less critical "warning only" level problems (P2)
  2. The recurrent "getaddress postcode lookup" check should be relegated to a P2 check
  3. The basic microservice healthcheck should not check dependencies. It is there only to allow CF components to verify that this service has mounted its router and is available to serve requests, regardless of the health of its dependencies.
  4. Removal of this and the beforeEach() antipattern from unit tests
  • Has the branch been rebased to develop?
  • Automated tests (Any of the following when applicable: Unit, Functional or Acceptance)
  • Manual compatibility testing (Browsers: Chrome, Firefox, IE11, Safari)

@data-hub-bot data-hub-bot temporarily deployed to data-hub-frontend-dev-pr-2313 December 9, 2019 11:55 Inactive
@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #2313 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2313      +/-   ##
===========================================
- Coverage    89.78%   89.77%   -0.01%     
===========================================
  Files          324      324              
  Lines         5569     5567       -2     
===========================================
- Hits          5000     4998       -2     
  Misses         569      569
Impacted Files Coverage Δ
src/apps/healthcheck/controllers.js 95.65% <100%> (-0.35%) ⬇️

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 1a0e207...c454784. Read the comment docs.

Copy link
Contributor

@web-bert web-bert 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, apart from the lists of services that can be cached.

@@ -1,231 +1,298 @@
const proxyquire = require('proxyquire')

const _successDependencies = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: using _ notation for private is not something I have seen across the rest of the codebase

src/apps/healthcheck/controllers.js Outdated Show resolved Hide resolved
src/apps/healthcheck/controllers.js Outdated Show resolved Hide resolved
Copy link
Contributor

@web-bert web-bert left a comment

Choose a reason for hiding this comment

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

Based on lack of time I don't want to block this

Copy link
Contributor

@rafenden rafenden 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, added optional comment regarding prefixes for private variables.

src/apps/healthcheck/__test__/controllers.test.js Outdated Show resolved Hide resolved
@mcbhenwood mcbhenwood temporarily deployed to data-hub-frontend-dev-pr-2313 December 10, 2019 17:30 Inactive
@mcbhenwood mcbhenwood force-pushed the chore/separate-pingdom-p1-and-p2-alerts branch from 0fcc4ef to 687d87a Compare December 10, 2019 17:31
@mcbhenwood mcbhenwood temporarily deployed to data-hub-frontend-dev-pr-2313 December 10, 2019 17:31 Inactive
@mcbhenwood mcbhenwood force-pushed the chore/separate-pingdom-p1-and-p2-alerts branch from 687d87a to c454784 Compare December 12, 2019 10:14
@mcbhenwood mcbhenwood merged commit ed06d8f into develop Dec 12, 2019
@mcbhenwood mcbhenwood deleted the chore/separate-pingdom-p1-and-p2-alerts branch December 12, 2019 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants