Support environment variables for all supported system properties in a uniform way #126
Conversation
| <daisy.webservice-utils.version>2.2.2</daisy.webservice-utils.version> | ||
| <daisy.updater-wrapper.version>1.0.2-SNAPSHOT</daisy.updater-wrapper.version> | ||
| <daisy.webservice.version>2.1.3-SNAPSHOT</daisy.webservice.version> | ||
| <daisy.webservice-utils.version>3.0.0-SNAPSHOT</daisy.webservice-utils.version> |
There was a problem hiding this comment.
I do not see API change in the webservice module, am I correct that the major change is due to change in the web service payload?
There was a problem hiding this comment.
I don't remember TBH. Might be a mistake.
There was a problem hiding this comment.
I remember now. It's because I changed the Properties class into an enum.
|
|
||
| import java.util.Map; | ||
|
|
||
| public class Properties { |
There was a problem hiding this comment.
if this class isn't intended to be extended, better make it final already, and if it's only intended as a static helper, we can add a private constructor too.
There was a problem hiding this comment.
OK good point. I normally preferfinal abstract, is that OK too?
There was a problem hiding this comment.
final abstract is not allowed. Normally I just add abstract without final. So your suggestion (final + private contructor) is the only real solution.
| public class Properties { | ||
|
|
||
| private final static java.util.Properties systemProperties = System.getProperties(); | ||
| private final static Map<String,String> systemEnv = System.getenv(); |
There was a problem hiding this comment.
This works only if properties can't change dynamically. I think it's OK (?) but this should probably be documented in Javadoc?
There was a problem hiding this comment.
I didn't really care that much about dynamic changes when I wrote this. But if we can support it we probably should. So you say that the Properties object that System.getProperties() returns does not return a different value for getProperty() after a property was changed? In that case I should change it. And same question for System.getenv()?
There was a problem hiding this comment.
TBH I don't remember if System.getProperties() or System.getenv() are dynamic, so it needs to be checked first. If they aren't, then problem solved :)
There was a problem hiding this comment.
I checked, System.getProperties() is indeed dynamic. I'll add a comment. @rdeltour You meant that if they are, then problem solved, right?
in a uniform way. A utility function was added to read from both System.properties and System.env. If a property starts with "org.daisy.pipeline", the function first looks for an environment variable and falls back to a system property. Otherwise it looks only for a system property. For example, if the key is "org.daisy.pipeline.ws.host", an environment variable "PIPELINE2_WS_HOST" will have precedence over the system property.
bertfrees
force-pushed
the
environment-variables
branch
from
6146515 to
3f751b9
Compare
See issue daisy/pipeline-assembly#141