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

[APPINT-1340] adding health_check #184

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

Conversation

hhemanth
Copy link
Contributor

@hhemanth hhemanth commented Nov 19, 2018

Adding health-check using health-check gem in the connector engine.
So the routes for health_check will be

GET|POST /health_check(/:checks)(.:format)

I created an connector-test app and have verified it.

Added Custom health check for testing connectivity to Connec! and Dev Platform!.
I used Maestrano.auto_configure for Connectivity to Dev Platform, Let me know if there is a better way to do it.

Added Documentation here -https://maestrano.atlassian.net/wiki/spaces/DEV/pages/28114962/Getting+started

@ouranos ouranos requested review from ouranos and a team November 19, 2018 23:49
Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

Instead of installing it in the host app with the generator, could make it part of the engine?
See how it's setup in mno-enterprise

@hhemanth
Copy link
Contributor Author

@ouranos
I have updated the pull request and the health check has been moved to the engine.

Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

Just a few tweaks.

We should also add some documentation in this section https://maestrano.atlassian.net/wiki/spaces/DEV/pages/25427971/Create+a+connector+using+our+framework+-+Ruby+on+Rails to explain how to add health checks.

See this section for example

config.http_status_for_error_object = 500

# You can customize which checks happen on a standard health check
config.standard_checks = %w[site cache redis-if-present]
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove redis-if-present. See maestrano/mno-enterprise#795

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same issue is in mno-enterprise 4.0, you want me to raise a pull request to fix there as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what the PR I linked above is doing?

@@ -29,6 +29,7 @@ Gem::Specification.new do |s|
s.add_runtime_dependency('attr_encrypted', '~> 1.4.0')
s.add_runtime_dependency('autoprefixer-rails')
s.add_runtime_dependency('bootstrap-sass')
s.add_runtime_dependency 'health_check', '~> 2.4'
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ~> 2.8 (2.7 got a bug with the redis check)
3.0 is for Rails 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same issue is in mno-enterprise 4.0 & 5.0, you want me to raise a pull request to fix there as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

No that's fine, it should install 2.8 on new projects and it was manually locked on older projects.
It's just because we adding it here we might as well put the latest version

config.standard_checks = %w[site cache redis-if-present]

# You can set what tests are run with the 'full' or 'all' parameter
config.full_checks = %w[site cache custom redis-if-present sidekiq-redis-if-present]
Copy link
Contributor

Choose a reason for hiding this comment

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

For the custom check we might want to add a check to see if the connector can contact:

  • connec!
  • dev-platform

See https://github.com/maestrano/mno-enterprise/blob/5.0/api/app/models/mno_enterprise/health_check.rb and the health_check README regarding custom_check

- removed redis-if-present check
- using 2.8 for health_check gema s 2.7 has issues
- added custom checks to test connectivity to connec! and Dev Platform.
@@ -154,3 +154,4 @@ Style/RescueStandardError:
- 'app/models/maestrano/connector/rails/concerns/entity.rb'
- 'app/models/maestrano/connector/rails/concerns/organization.rb'
- 'app/resources/maestrano/api/base_resource.rb'
- 'app/models/maestrano/connector/rails/health_check.rb'
Copy link
Contributor

Choose a reason for hiding this comment

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

That shouldn't be needed?

# Check API connection with Connec!
# any code that returns blank on success and non blank string upon failure
def self.perform_connec_check
organization = Maestrano::Connector::Rails::Organization.first
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are no Organization?

# Check API connection with Dev Platform!
# any code that returns blank on success and non blank string upon failure
def self.perform_dev_platform_check
if Maestrano.auto_configure
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried it'll re-configure the connector at every health check request.
Could we just find a way to query the DevPlatform without re-configuring the connector, or maybe just checking that the connector is configured enough? (by looking at the Maestrano config?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants