DS-4189 Enhance configuration by environment variables

[pnbecker]

https://jira.duraspace.org/browse/DS-4189

To test:

export DSPACE__FOO=bar
/dspace/bin/dspace dsprop -p dspace.foo

[pnbecker]

Unfortunately this cannot be backported as it requires Apache Commons Configuration 2 which is available as of DSpace 7.

[terrywbrady]

I will give this a test and report my results. I have a couple initial comments.

If this could not be implemented in DSpace 6, I presume we will use another technique (probably sed ... > local.cfg) for DSpace 6. We can introduce different initialization scripts for each version of DSpace.

We have at least one variable that uses camel case dspace.baseUrl, so I do not think that we should translate upper case to lower case for property names.

[terrywbrady]

Aside from the lower-case issue I mentioned, this code looks good. I think this is the optimal approach for passing there properties into a container.

I tested locally and the changes work as expected.

For the earlier branches (DSpace 6, etc), we will implement a less-optimal solution for Docker purposes.

[pnbecker]

I‘ll change this to be case sensitive. That is an easy task.
@terrywbrady I can check again if this really depends on Apache Commons Config 2. the first version was for sure, but it changed a lot since.

[terrywbrady]

Fyi... I propose the following changes to our docker compose repo to accompany this change.

DSpace-Labs/DSpace-Docker-Images#97

[pnbecker]

@terrywbrady @tdonohue I made the changes requested. The configuration now works case sensitive. I tested it with dspace.baseURL and dspace.baseUrl and dspace.name.

[terrywbrady]

Since my last review, I discovered that we also need a mechanism to set properties containing a dash. I was attempting to set sword-server.enabled=true and I discovered a similar issue to the one we discovered with periods.

I propose that we map __dash__ to -.

I would be open to a different mapping.

The following PR illustrates why this additional enhancement will be needed.

DSpace-Labs/DSpace-Docker-Images#98

[tdonohue]

The code here is looking good overall. Since @terrywbrady points out there are additional problem characters, we may want to come up with a more standard replacement strategy (which could cover other problem characters, if any arise). So, as suggested by @terrywbrady , we could consider a standard replacement format like:

• __dash__ to -
• __dot__ to .

As-is, this PR is definitely specific to Apache Commons Configuration v2, as it relies on the concept of ConfigurationBuilders (which was introduced in that release, see Migrating to 2.0). So, I'm not sure this can be backported -- but it seems like a good solution for DSpace 7.x and above.

[pnbecker]

I'm not a friend of so long environment variable names: sword__dash__server__dot__enabled, dspace__dot__name (even if we would decide to go by one underline only).
How many properties do we have that contain any other characters than letters, numbers, dots and underlines? How many properties do we have that contain dashes? Could we think about renaming a few for DSpace 7 and create a rule to use letters, numbers, dots and dashes only?

[terrywbrady]

I do not think we should rename properties. I think we should keep the property names constant across DSpace versions

I agree that my first proposal would create long keys. We could go with ___ -> - and __ -> . or _P_ -> . and _D_ -> - (Period and Dash).

[tdonohue]

Just a quick note. I agree here with @terrywbrady. I don't think we should rename properties to please Docker. I'd rather we come up with a reasonable replacement strategy.

While I definitely understand the desire to keep environment variables short, I think we also need to consider the fact that many (most?) people using Docker may not even use these environment variables (beyond the default ones pre-set for them to initialize their Docker environment). So, readability/length of the environment variables may not be a major concern if most users are not even aware of them.

[terrywbrady]

We had a quick chat on Slack and decided to go with the following replacement code for consistency.

• __D__ -> -
• __P__ -> .

I have applied changes addressing my prior review

[terrywbrady]

Output to demonstrate that the change is in place

$ winpty docker exec -it dspace //bin/bash
root@a9d8eefce1d3:/usr/local/tomcat# env|grep __
swordv2__D__server__P__path=swordv2
dspace__P__dir=/dspace
swordv2__D__server__P__collection__P__url=http://localhost:8080/swordv2/collection
swordv2__D__server__P__enabled=true
swordv2__D__server__P__url=http://localhost:8080/swordv2
dspace__P__hostname=localhost
dspace__P__baseUrl=http://localhost:8080
sword__D__server__P__enabled=true
dspace__P__name=DSpace Started with Docker Compose
swordv2__D__server__P__servicedocument__P__url=http://localhost:8080/swordv2/servicedocument
db__P__url=jdbc:postgresql://dspacedb:5432/dspace
root@a9d8eefce1d3:/usr/local/tomcat# cat /dspace/config/local.cfg
# ------------------------------------------------------------------------
# This file contains the localized properties for published DSpace images.
# See https://github.com/DSpace-Labs/DSpace-Docker-Images for usage information.
# ------------------------------------------------------------------------
dspace.dir = /dspace
db.url = jdbc:postgresql://dspacedb:5432/dspace
dspace.hostname = localhost
dspace.baseUrl = http://localhost:8080
root@a9d8eefce1d3:/usr/local/tomcat# /dspace/bin/dspace dsprop -p sword-server.enabled
true
root@a9d8eefce1d3:/usr/local/tomcat# /dspace/bin/dspace dsprop -p swordv2-server.enabled
true

[terrywbrady]

@tdonohue , @mwoodiupui , @pnbecker , could you give this a quick review?

[tdonohue]

👍 Overall, this looks good. One minor inline note below about correcting an inline comment to better document the behavior of the new dspaceenv tag in config-definition.xml

[mwoodiupui]

+1 by inspection.