Stream: ibm
Topic: issue #1517 - env vars in fhir-server-config
Lee Surprenant (Oct 05 2020 at 15:57):
We got a really nice external contribution for adding this feature that will improve deployment automation for fhir-server in a modern architecture: https://github.com/IBM/FHIR/pull/1552
Basically the PR implements an approach that I had outlined where we us Apache StringSubstitutor to replace values in the config file with values from environment variables.
I think there is only one outstanding discussion on this PR and we wanted to call attention to it / open it up to the wider team: https://github.com/IBM/FHIR/pull/1552#discussion_r498413614
Lee Surprenant (Oct 05 2020 at 21:27):
@Paul Bastide I've read and thought your arguments on this one but I'm leaning towards NOT forcing the prefix/pattern for env var substitutions. The main thing swaying me is that neither Liberty (server.xml), Docker, nor Kubernetes seem to impose any such pattern/limitation on environment variables that can be set / configured.
Do you have any pointers to common / adopted systems that do enforce such a pattern?
How strong do you feel about the security benefits of forcing the prefix?
Lee Surprenant (Oct 05 2020 at 21:29):
the other thing swaying me is that the external contributor seemed to feel strongly enough to argue about not forcing the prefix. i realize this isn't a great reason, but I really want to encourage external contributors like this person and, if the security impact is negligable, I'd give the "benefit of the doubt" to how it was coded up in the PR
Paul Bastide (Oct 05 2020 at 23:40):
I'm personally against it. I think it's a bad pattern that pollutes the entire namespace. leading to side-effects, for instance look at so many antipatterns with namespaces and the concepts of isolating concerns. Also, take a peek at environment variables themselves in Java. I defer to you and John. to break the tie. I don't think this is a good pattern and makes it near impossible to consistently isolate concerns. These are my ten cents and more.
Lee Surprenant (Oct 06 2020 at 15:47):
Looks like John added his 2 cents on the PR as well. Having noted the dissenting opinion, I think we're set on this one. Ready to merge?
Paul Bastide (Oct 07 2020 at 16:53):
definitely
Last updated: Apr 12 2022 at 19:14 UTC
