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.

Sign up for GitHub

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

Jump to bottom

Upgrade SmallRye Health to 2.2.3 and add Health UI #10828

Merged
merged 1 commit into from
Jul 21, 2020
Merged

Upgrade SmallRye Health to 2.2.3 and add Health UI #10828

merged 1 commit into from
Jul 21, 2020

Conversation

phillip-kruger
Copy link
Member

This PR upgrade the SmallRye Health to the latest 2.2.3 version and also add the new Health UI

Signed-off-by: Phillip Kruger [email protected]

@boring-cyborg boring-cyborg bot added area/dependencies Pull requests that update a dependency file area/smallrye labels Jul 18, 2020
@phillip-kruger
Copy link
Member Author

@xstefank - please have a look

xstefank
xstefank approved these changes Jul 20, 2020
View reviewed changes
Copy link
Member

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

Discussed with @phillip-kruger offline and everything was explained. Approving.

@phillip-kruger phillip-kruger requested review from gastaldi and removed request for kenfinnigan July 20, 2020 13:14
@gastaldi
Copy link
Contributor

This is awesome, but comparing to openapi/swagger-ui, wouldn't be worth moving health-ui to its own extension?

@phillip-kruger
Copy link
Member Author

Hi @gastaldi - I actually want to do the reverse :) Move swagger-ui to openapi. We included this UI similar to the GraphQL UI

@gastaldi
Copy link
Contributor

This makes sense to me, as they are directly related. +1 to that

@phillip-kruger phillip-kruger requested review from gastaldi and gsmet July 20, 2020 14:14
@gastaldi
Copy link
Contributor

Can we have some documentation in the microprofile-health.adoc about this new feature? Make sure to include the new configuration parameters there too

@phillip-kruger
Copy link
Member Author

@gastaldi - Yes, either myself or @xstefank will do that in a separate PR.

gastaldi
gastaldi approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be nice to have the documentation too in this same PR if possible (could be in a separate commit)

@phillip-kruger
Copy link
Member Author

@xstefank - w.d.y.t ? Do you want to update the documentation, or can I give it a shot ?

@kenfinnigan
Copy link
Member

Not a blocker, but we need to rethink the documentation from a SmallRye perspective.

For instance, talking about the Health UI doesn't really belong in a doc about MP Health.

Maybe it's just the file name, but we need to be clear about what is MP and what isn't

@agoncal
Copy link
Contributor

agoncal commented Jul 20, 2020

@phillip-kruger in Quarkus we can change Swagger UI path, enable it or not, etc. with the following properties. Do you think of doing the same ?

  • quarkus.swagger-ui.path: The path where Swagger UI is available.
  • quarkus.swagger-ui.always-include: If this should be included every time. By default this is only included when the application is running in dev mode
  • quarkus.swagger-ui.enable: If Swagger UI should be enabled. By default, Swagger UI is enabled.

@phillip-kruger
Copy link
Member Author

phillip-kruger commented Jul 20, 2020 •
edited
Loading

Hi @agoncal .

Yes you already can - it follows the same pattern as swagger-ui (and graphiql):

quarkus.smallrye-health.ui.root-path
quarkus.smallrye-health.ui.always-include
quarkus.smallrye-health.ui.enable

We will update the documentation soon.

@phillip-kruger
Copy link
Member Author

@gastaldi - I added the documentation as requested.

@xstefank
Copy link
Member

We have SR Health documentation on the roadmap and I think that Quarkus documenation should then point out to that when its ready. However, the doc is really about MP Health and mentioning additional features only available in SR/Quarkus may be confusing for someone who reads this doc as first thing as an intro to the spec. I would suggest to add some mention that UI is not part of the spec.

@phillip-kruger
Copy link
Member Author

Thanks @xstefank - Can you suggest where/what in the PR ?

@phillip-kruger phillip-kruger requested a review from gastaldi July 21, 2020 10:48
gastaldi
gastaldi approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

Tested locally. Works great! Good job @phillip-kruger

@gastaldi gastaldi merged commit 96ece8e into quarkusio:master Jul 21, 2020
@phillip-kruger phillip-kruger deleted the smallrye-health-update branch July 21, 2020 13:54
@phillip-kruger phillip-kruger mentioned this pull request Jul 21, 2020
Merged
@gsmet gsmet added this to the 1.7.0 - master milestone Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xstefank xstefank xstefank approved these changes

@gastaldi gastaldi gastaldi approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/dependencies Pull requests that update a dependency file area/documentation area/smallrye
Projects
None yet
Milestone
1.7.0.CR1
Development

Successfully merging this pull request may close these issues.
6 participants
@phillip-kruger @gastaldi @kenfinnigan @agoncal @xstefank @gsmet