7000 mpconfig fqdn & siteUrl #8826
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
poikilotherm
added
Component: Code Infrastructure
This was referenced Jun 30, 2022
…CONFIG IQSS#7000 - Add both settings to JvmSettings to enable lookup - Refactor SystemConfig.getDataverseSiteUrlStatic to use MPCONFIG, but keep current behaviour of constructing the URL from FQDN or DNS reverse lookup. (Out of scope here, see IQSS#6636) - Replace clones of the method in Xrecord, DdiExportUtil, HandlenetServiceBean with direct usages of the static method to avoid unnecessary duplicated code. - Refactor SchemaDotOrgExporterTest with @JvmSetting for site url. - Remove unused constants from SystemConfig - Added default for container usage within "ct" profile, so we avoid extra lookups/settings for development usage. See also IQSS#6636
- Notes about MPCONFIG usage. - Rewording to make it more clear how this shall be used.
poikilotherm
force-pushed
the
7000-mpconfig-fqdn
branch
from
a58e0bd to
3f19c12
Compare
For unknown reasons, the test assumed the site url / fqdn to be "https://librascholar.org", which might be coming from some test order side effect. Now the test sets the site URL setting to have control over the generated data. On a related note, this meant to upgrade the test from JUnit4 to JUnit5 plus some minor code cleanups.
|
added to sprint Dec 15, 2022 |
|
@poikilotherm can you please resolve the merge conflicts? Thanks! |
I did! 😸 |
|
I'm running "build now" in Jenkins, hoping that this error goes away: java.lang.AssertionError: expected:<201> but was:<500> |
pdurbin
approved these changes
Dec 19, 2022
There was a problem hiding this comment.
I didn't test this but the code changes make sense, the docs look good, and all API tests are passing (I did have to re-run them due to the new harvest client tests). Sending to QA. Thanks, @poikilotherm.
|
Woo-hoo! Thanks y'all, as always! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This pull request is simply swapping the former retrieval of the setting for FQDN and/or Site URL to MPCONFIG.
The code has been de-duplicated in some places that had the fqdn/siteUrl logic repeated. Those dups have been replaced with SystemConfig calls.
This PR does not address the changes requested in #6636 by request of @pdurbin for a smaller scope and less brain overload.
As this PR reuses existing config settings and does no renaming, this is 100% backward compatible.
Nonetheless, I really tried to make the description of the two settings in the config guide more comprehensible.
Relates to #6636
Relates to #7000
Which issue(s) this PR closes:
None.
Special notes for your reviewer:
It is again a good and simple start to see how the pieces laid out in #8823 are coming together.
Suggestions on how to test this:
Play around with the settings described in the added docs and watch it work. You could try an env var for a change (although in a classic installation, these might be hard to change for the Payara appserver...)
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Not really.
Is there a release notes update needed for this change?:
Maybe? Dunno. The actual behaviour did not change as requested, this is not a solution to #6636.
Additional documentation:
Included.