Implement Kibana/OpenSearch Dashboards dev services #51164
Conversation
quarkus-bot
bot
added
area/dependencies
|
/cc @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch) |
marko-bekhta
left a comment
marko-bekhta
left a comment
There was a problem hiding this comment.
Hey 👋🏻
thanks for the PR! 🙂
Could you please use rebase instead of adding merge commits? Something like:
git checkout main
git fetch upstream
git rebase upstream/main
where upstream points to this repo (git remote add upstream https://github.com/quarkusio/quarkus.git that is if you haven't set up the upstream yet) usually should do it for your current branch (main from which you've opened this PR) 🙂
I think it would be better if you just create another processor for the dashboards (i.e. DevServicesElasticsearchDashboardsProcessor), that way you'd easiely spot if you are not missing any stop/restart actions + the processor for the Elasticsearch services is about to change to work with multiple services at the same time.
You've made good progress here already 👍🏻
Have you had a chance to look into this part about dev ui as well:
it would be a great start if we provided either an option to launch Kibana in the Elasticsearch dev services (and include a link to it in the Dev UI),
it can go as a follow up if not 🙂
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Show resolved
Hide resolved
| for (String hostsConfigProperty : buildItemConfig.hostsConfigProperties) { | ||
| // Check if elasticsearch hosts property is set | ||
| if (ConfigUtils.isPropertyNonEmpty(hostsConfigProperty)) { | ||
| log.debugf("Not starting Dashboard Dev Services for Elasticsearch, the %s property is configured.", | ||
| hostsConfigProperty); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
if there are host properties set, it means we will be woking with a provided ES cluster, but we can still start a dev service for kibana and let the dev use that to connect to that cluster, no ?
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
...io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java
Outdated
Show resolved
Hide resolved
| private static Properties loadProperties(String devserviceName) { | ||
| var fileName = devserviceName + "-devservice.properties"; | ||
| try (InputStream in = Thread.currentThread().getContextClassLoader().getResourceAsStream(fileName)) { | ||
| if (in == null) { | ||
| throw new IllegalArgumentException(fileName + " not found on classpath"); | ||
| } | ||
| var properties = new Properties(); | ||
| properties.load(in); | ||
| return properties; | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Let's keep this in the utils... instead we should just use the corresponding name for the properties file e.g. kibana-devservice.properties / opensearch-dashboards-devservice.properties
| @@ -1 +1,2 @@ | |||
| default.image=${elasticsearch.image} | |||
| default.dashboard.image=${kibana.image} | |||
There was a problem hiding this comment.
See the other comment, this will probably go into its own properties file.
|
Thanks for all your comments. So I would move all my changes into a separate class DevServicesDashboardProcessor with its own properties files and address your proposed changes. Regarding the Dev Ui. Yes I had that in mind but I planned it as a follow up. |
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error when shutting down dashboard dev service as proposed by Marko Co-authored-by: Marko Bekhta <marko-bekhta@users.noreply.github.com>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error as proposed by Marko. Co-authored-by: Marko Bekhta <marko-bekhta@users.noreply.github.com>
Fixed duplicate Kibana image prop as proposed by Marko. Co-authored-by: Marko Bekhta <marko-bekhta@users.noreply.github.com>
…n/java/io/quarkus/elasticsearch/restclient/common/deployment/DevServicesElasticsearchProcessor.java Fixed copy paste error as proposed by Marko. Co-authored-by: Marko Bekhta <marko-bekhta@users.noreply.github.com>
4 participants
This change will add dashboard services to the dev services for extensions/elasticsearch-rest-client and extensions/elasticsearch-java-client. If the distribution is elasticsearch, it will start Kibana as the dashboard. If the distribution is opensearch, it will start OpenSearch-dashboards as the dashboard.