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

feat(health): add option to enable/disable openapi spec #6645

Merged
merged 1 commit into from Oct 27, 2020

Conversation

nflaig
Copy link
Contributor

@nflaig nflaig commented Oct 25, 2020

Adds the option to enable the openapi spec which allows to show the endpoints in the API explorer. By default it will be disabled.

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated

extensions/health/src/types.ts Outdated Show resolved Hide resolved
extensions/health/src/types.ts Outdated Show resolved Hide resolved
@nflaig
Copy link
Contributor Author

nflaig commented Oct 26, 2020

@raymondfeng should the HealthChecker and HealthObserver be added to the application if the options.disabled is true?

Also one more thing, does it make sense to add 500 and 503 to the responses in the spec or is just the 200 response enough?

@raymondfeng
Copy link
Contributor

@nflaig nflaig force-pushed the feat/openapi_spec branch 2 times, most recently from 9580446 to 4eba30f Compare October 27, 2020 16:22
@raymondfeng
Copy link
Contributor

should the HealthChecker and HealthObserver be added to the application if the options.disabled is true?

Interesting, if we decide to disable health component, we can simply remove app.component(HealthComponent). So far the behavior of disabled: true is to not expose the http endpoints. I think we can leave it out of this PR.

Signed-off-by: nflaig <nflaig@protonmail.com>
@nflaig
Copy link
Contributor Author

nflaig commented Oct 27, 2020

Interesting, if we decide to disable health component, we can simply remove app.component(HealthComponent).

I agree that one could just put a if-statement around the app.component(HealthComponent) to disable it through the config, however it does feel like the cleanest approach to me. But I guess this also depends on how the configuration in the app that is using the component is handled in general.

I think we can leave it out of this PR

agreed

@raymondfeng raymondfeng merged commit 7227ab7 into loopbackio:master Oct 27, 2020
2 checks passed
@raymondfeng
Copy link
Contributor

@nflaig One more PR landed. Great job! 🥇

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.

None yet

3 participants