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

[WFLY-12779] Incorporate MicroProfile Config in RESTEasy #262

Merged
merged 7 commits into from
May 29, 2020

Conversation

ronsigal
Copy link
Contributor

No description provided.

@ronsigal ronsigal changed the title [wFLY-12779] Incorporate MicroProfile Config in RESTEasy [WFLY-12779] Incorporate MicroProfile Config in RESTEasy Nov 12, 2019

=== Testing By

[x] Engineering

Choose a reason for hiding this comment

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

This RFE should be tested also by QE, asking for bump of the rank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@ronsigal
Copy link
Contributor Author

ronsigal commented Dec 26, 2019

  1. As I mentioned on WFLY-12779, I would like to re-consider the introduction of parameter "resteasy.config.resteasy.parameters.only", which would restrict the use of RESTEasy's servlet specific ConfigSources to returning only RESTEasy parameters. Thinking about it again, it seems strange to allow MicroProfile Config users to access any parameters using the other ConfigSources and then impose a restriction just on RESTEasy ConfigSources. I would like to dispense with that idea.
  1. I have submitted a RESTEasy pull request ([RESTEASY-2406] Port use of Microprofile Config to 3.x resteasy/resteasy#2250) with all of the appropriate changes. It should be merged into RESTEasy 3.10.0.Final (, which should then be upgraded in Wildfly 19 (https://issues.redhat.com/browse/WFLY-12712).

  2. [RESTEASY-2406] Port use of Microprofile Config to 3.x resteasy/resteasy#2250 includes several tests (https://github.com/ronsigal/Resteasy/tree/3.10_2406/testsuite/integration-tests/src/test/java/org/jboss/resteasy/test/microprofile/config). These tests verify that configuration information can be retrieved from system variables, servlet context parameters, servlet init parameters, and web.xml filter parameters and that the ConfigSources are accessed in the proper order according to their assigned ordinals.

  3. [RESTEASY-2406] Port use of Microprofile Config to 3.x resteasy/resteasy#2250 also includes relevant documentation in the RESTEasy User Guide:https://github.com/ronsigal/Resteasy/blob/3.10_2406/docbook/reference/en/en-US/modules/Installation_Configuration.xml#L445. The release 4.x version of the same documentation can be read here: https://docs.jboss.org/resteasy/docs/4.4.2.Final/userguide/html/Installation_Configuration.html#microprofile_config

In RESTEasy 4.x, all parameters, both RESTEasy parameters and application parameters, are
available from all ConfigSources to both RESTEasy and application code. In RESTEasy 3.x, we
propose to add a RESTEasy parameter, "resteasy.config.resteasy.parameters.only", which, when
set to "true", will limit the three RESTEasy ConfigSources to returning RESTEasy parameters
Copy link

Choose a reason for hiding this comment

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

What parameters would be actually excluded? Can you outline an use-case?

Copy link

Choose a reason for hiding this comment

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

@ronsigal I find that confusing as well. Do I understand it correctly, when resteasy.config.resteasy.parameters.only=true

* servlet init-params [ordinal 60]
* filter init-params [ordinal 50]
* servlet context-params [ordinal 40]

parameters will be exposed only to RestEasy library? Why not to have all parameters available everywhere? Is it to keep backward compatibility of servlet context-params behaviour? Was there before this RFE way for application to consume servlet context-param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed reference to "resteasy.config.resteasy.parameters.only"

@ronsigal
Copy link
Contributor Author

Hi @istraka, @mchoma,

Actually, I backtracked on that, but I see I mentioned it on WFLY-12779, rather than EAP7-1386:

One thing I would like to reconsider is the proposal to implement the parameter "resteasy.config.resteasy.parameters.only". Thinking more about it, it seems awkward to restrict the scope of some ConfigSources when most of them, the default ConfigSources as well as those added in Wildfly, are out of RESTEasy's control. I'm thinking now that it makes more sense to allow any parameters to be retrieved in RESTEasy.

Sorry for the confusion. I'll update the document.

1. Removed reference to "resteasy.config.resteasy.parameters.only"
2. Moved testing to QE

=== Hard Requirements

* Update RESTEasy to retrieve all configuration parameters from MicroProfile ConfigSources.

Choose a reason for hiding this comment

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

Could you please elaborate that this one will apply only in case that MicroProfile classes are available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pkremens,

I've rewritten the proposal to account for the fact that RESTEasy must be able to function with or without the availability of MicroProfile Config.

-Ron

Copy link

Choose a reason for hiding this comment

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

Hi @ronsigal

thanks! It is a public holiday today here in Czech Republic so I'll take a look on Monday.

Copy link

Choose a reason for hiding this comment

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

Hi @ronsigal,
thanks for updates, looking good to me.

I don't think we need to extend the Test Plan section of this document more, since there will be a separate document in "jbossqe-eap/test-plans" covering that part (we can mention a new profile disable.microprofile.tests also in there, as it will be needed only for EAP testing).

@jamezp jamezp merged commit 5e412dc into wildfly:master May 29, 2020
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.

6 participants