Conversation
poikilotherm
added
Type: Feature
poikilotherm
force-pushed
the
7457-introduce-mpconfig
branch
from
8790313 to
b1ea846
Compare
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
I saw this was in review so I took a quick look. I didn't run the suggested verify command. It sounds like more docs are coming, which is good.
I think I'm still a little confused about the "why". There's nice work here but what does it mean for people who run or develop Dataverse? What problems does it solve? I'm sure in time, all will be revealed. 😄
There was a problem hiding this comment.
Can we get this to pass on Jenkins?
I'm seeing this error:
[ERROR] Errors:
[ERROR] AliasConfigSourceTest.getValue:25 » IllegalState javax.naming.NoInitialContext...
[ERROR] AliasConfigSourceTest.readImportTestAliasesFromFile:39 » IllegalState javax.na...
over at https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-7463/4/console
(mvn -P tc verify worked fine on my laptop.)
Also, while I'm in favor of small chunks generally, I'm eager to see how a fix for #7424 makes use of this groundwork, to make it more real. If it's possible to get a preview of that future pull request, it would be nice.
… JNDI could not be found or injected. Will result empty properties for this source. IQSS#7457
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
This pull request doesn't seem to do any harm and the API test suite passes so I'm moving it to QA.
That said, other developers are quite welcome to take a look.
My overall take on this is that @poikilotherm is trying to move us toward modern technologies like the MicroProfile Config API which will ultimately make sysadmins happier with their options for configuring the application.
I did note the following from @poikilotherm in IRC but again, since the API test suite passed, deployment must have succeeded so I think we're all set.
Doh! I found the troubles with 7457 not deploying. It's just the same thing that broke tests, too. OK will need to transform the source into a bean and inject the datasource...
|
Sorry @pdurbin dragging this back to dev. I need to work around the dependency injection obstacles first. Will push soon. |
…jected by a singleton on startup.
… has not been injected yet, but values are requested already.
|
@donsizemore @pdurbin Jenkins fails with non-reachable SSH. I'm sure this isn't related to my changes. What now? |
|
@poikilotherm I just clicked "build now" on Jenkins. Meanwhile, I'll check out the further tweaks to the docs. Do you have any idea why they won't build? (And can you fix it?) Check this out:
|
Sorry @pdurbin this slipped through. Was a bit early this morning. Whitespace removed, Sphinx happy. |
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
I'm going to move this to QA but again, I'd be happy for another developer to take a look.
Question for @poikilotherm ... will all this fancy config stuff helps us solve #6953? 😄 I hope so!
5 participants
What this PR does / why we need it:
This PR paves the ground to finally enable using MicroProfile Config API within the application.
By providing two special config sources, we can rename settings but still stay backward compatible.
We can also start to migrate database settings to MPCONFIG where we see fit and still be backward compatible.
It will log using a deprecated setting, so we can stay with a setting for a while before removing it.
Which issue(s) this PR closes:
Closes #7457
Special notes for your reviewer:
None.
Suggestions on how to test this:
Actually this is not introducing a change, as we don't have aliases yet nor are we migrating a database setting here. This will be done for #7424 (PR forthcoming).
For now, you can take a look at the added unit tests and running a testcontainers test via
mvn -P tc verify.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Nope.
Is there a release notes update needed for this change?:
🔋 included.
Additional documentation:
None so far.