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

Extension to view MicroProfile config values in dev mode #5692

Closed
wants to merge 3 commits into from
Closed

Extension to view MicroProfile config values in dev mode #5692

wants to merge 3 commits into from

Conversation

hpehl
Copy link

@hpehl hpehl commented Nov 22, 2019

PR for #1349.

Adds a servlet in dev mode which lists the configured values for MicroProfile Configuration. The servlet is registered using the path /config by default and lists all properties of all config sources. The config sources are sorted descending by ordinal, the properties by name. If no config is defined an empty JSON object is returned.

There's no integration test in this PR. Since the servlet is only added in dev mode, @QuarkusTest does not work as expected.

If necessary I can provide documentation.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@hpehl hpehl changed the title Quarkus config viewer extension Extension to view MicroProfile config values in dev mode Nov 22, 2019
Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

I'm not sure if we need some extension metadata to indicate that this is dev only, if that's possible?

@@ -87,7 +88,6 @@
<module>spring-di</module>
<module>spring-web</module>
<module>spring-data-jpa</module>
<module>spring-security</module>
Copy link
Member

Choose a reason for hiding this comment

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

are these modules meant to be removed?

Copy link
Author

Choose a reason for hiding this comment

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

Should be fixed now.

* }
* </pre>
*/
@WebServlet
Copy link
Member

Choose a reason for hiding this comment

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

Ideally I think this should be a pure Vert.x Handler like Health and Metrics: https://github.com/quarkusio/quarkus/blob/master/extensions/smallrye-health/runtime/src/main/java/io/quarkus/smallrye/health/runtime/SmallRyeHealthHandler.java

And thus change the dependency from undertow to vertx. That's going to be the lowest common denominator

Copy link
Author

Choose a reason for hiding this comment

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

I'm using a pure Vert.x handler now.


import org.eclipse.microprofile.config.Config;

public class ConfigHolder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be needed IIUC, you could just pull Config out of CDI.current() instead of wrapping in this class

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll try to simplify that.

Copy link
Author

Choose a reason for hiding this comment

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

Removed ConfigHolder.

@geoand
Copy link
Contributor

geoand commented Nov 22, 2019

Looks nice!

I am wondering if this could be part of the standard dev mode experience without the user having to add another extension to have access to the view.

@geoand
Copy link
Contributor

geoand commented Nov 29, 2019

@hpehl did you perhaps look into having this available in dev mode by default without requiring the user to add an extra dependency?

@hpehl
Copy link
Author

hpehl commented Nov 29, 2019

Sounds reasonable.
How would that work? Still as an extension or do I need to integrate it in a different way?

@geoand
Copy link
Contributor

geoand commented Nov 29, 2019

I was think that if the code could live in the io.quarkus:quarkus-development-mode module that would be great since it wouldn't require users to add anything new

@hpehl
Copy link
Author

hpehl commented Nov 29, 2019

Ok. I'm going to refactor and update the PR.

@geoand
Copy link
Contributor

geoand commented Nov 29, 2019

@hpehl thanks!

Best have the refactoring in a separate commit in case someone else comes up with a good reason why this change shouldn't be in the core devmode code.

Thanks

@hpehl
Copy link
Author

hpehl commented Dec 10, 2019

@geoand Adding the code directly in quarkus-development-mode would add dependencies to quarkus-vertx-http and quarkus-jsonp. Not sure if that's what we want?

DevModeMain.java:167

String configPath = buildSystemProperties.getProperty("quarkus.config.path", "/config");
builder.addChainCustomizer(new Consumer<BuildChainBuilder>() {
    @Override
    public void accept(BuildChainBuilder buildChainBuilder) {
        buildChainBuilder.addBuildStep(new BuildStep() {
            @Override
            public void execute(BuildContext context) {
                // this would add dependencies to quarkus-vertx-http and quarkus-jsonp
                context.produce(new RouteBuildItem(configPath, new ConfigViewerHandler(), HandlerType.BLOCKING));
            }
        });
    }
});

Are there other ways to get this into quarkus-development-mode?

@gastaldi gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Dec 11, 2019
@gsmet
Copy link
Member

gsmet commented Feb 21, 2020

@geoand should we try to get that one into 1.3? It looks useful.

@geoand
Copy link
Contributor

geoand commented Feb 21, 2020

Yeah it would definitely be nice to have.

vertx-http (or perhaps it's internal version) should already be a dependency of the dev-mode, so I don't think that should be a problem.
Adding jsonp I would like to avoid if at all possible...

@hpehl
Copy link
Author

hpehl commented Feb 22, 2020

Thanks for the feedback @gsmet and @geoand. I'll try to update the PR to the latest versions.

@geoand
Copy link
Contributor

geoand commented Feb 26, 2020

So should this be closed now?

@hpehl
Copy link
Author

hpehl commented Feb 26, 2020

Superseded by #7430

@hpehl hpehl closed this Feb 26, 2020
@hpehl hpehl deleted the config-viewer-extension branch February 26, 2020 18:42
@gsmet gsmet added the triage/invalid This doesn't seem right label Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right triage/needs-rebase This PR needs to be rebased first because it has merge conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants