Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some properties are hard to reference from environment #178

Closed
petermicuch opened this issue Nov 30, 2020 · 8 comments
Closed

Some properties are hard to reference from environment #178

petermicuch opened this issue Nov 30, 2020 · 8 comments

Comments

@petermicuch
Copy link
Contributor

It is difficult to easily reference some properties of starter hapi server after switch to spring boot and related switch from hapi.properties file to application.yaml. Especially it is very difficult for arrays such as hapi.fhir.tester.

According to relaxed binding guide and binding from environment structure that the current application.yaml has, should actually work. However there is one case that is not described even in spring boot documentation and that is the case of arrays read from environment variables.

In the documentation of spring boot, you can read: To convert a property name in the canonical-form to an environment variable name you can follow these rules:

  • Replace dots (.) with underscores (_).
  • Remove any dashes (-).
  • Convert to uppercase.
  • Replace array indexes [x] with _x_, where x is integer.

By following above rules, let's convert this snippet from application.yaml:

hapi:
  fhir:
    tester:
      -
          id: home
          name: Local Tester
          server_address: 'http://localhost:8080/fhir'
          refuse_to_fetch_third_party_urls: false
          fhir_version: R4

into environment variables:

HAPI_FHIR_TESTER_0_ID=home
HAPI_FHIR_TESTER_0_NAME="Local Tester"
HAPI_FHIR_TESTER_0_SERVER_ADDRESS=http://localhost:8080/fhir
HAPI_FHIR_TESTER_0_REFUSE_TO_FETCH_THIRD_PARTY_URLS=false
HAPI_FHIR_TESTER_0_FHIR_VERSION=R4

However except of HAPI_FHIR_TESTER_0_ID and HAPI_FHIR_TESTER_0_NAME nothing is correctly bound to respective variables. Experimenting with these bindings for a bit I found out that after removing underscores _ from env variable names after array index, it started to work fine.
Working example:

HAPI_FHIR_TESTER_0_ID=home
HAPI_FHIR_TESTER_0_NAME="Local Tester"
HAPI_FHIR_TESTER_0_SERVERADDRESS=http://localhost:8080/fhir
HAPI_FHIR_TESTER_0_REFUSETOFETCHTHIRDPARTYURLS=false
HAPI_FHIR_TESTER_0_FHIRVERSION=R4

Considering spring boot documentation recommendation and above undocumented behavior, I personally think that the best for hapi starter project would be to switch from underscore notation to either camel case or kebab case notation (i.e. hapi.fhir.tester.serverAddress or hapi.fhir.tester.server-address). To preserve more or less same readability of application.yaml file, kebab case notation is probably more suitable although my personal preference is camel case in configuration as well as naming in the code itself. But no matter if camel case or kebab case is chosen, once you are following rules given in binding from environment guide all simply works without issues.

@jvitrifork or @jamesagnew - could you please consider this to better support setting from environment, which is quite common and more flexible than application.yaml file loaded from configuration map. Thank you.

@jvitrifork
Copy link
Collaborator

As mentioned on #164 - the PR on #177 should fix most of this. The syntax for adressing the tester using docker (more or less directly transferrable to kubernetes) would eg. be docker run -p 8080:8080 -e HAPI_FHIR_FHIR_VERSION=DSTU3 -e HAPI_FHIR_TESTER_MEDCOM_NAME=Medcom -e HAPI_FHIR_TESTER_MEDCOM_SERVER_ADDRESS="http://fhir.medcom.dk:8080/fhir" -e HAPI_FHIR_TESTER_MEDCOM_FHIR_VERSION=R4 hapi.

Now, with the guide you have outlined from (https://docs.spring.io/spring-boot/docs/current/reference/html/spring-boot-features.html#boot-features-external-config-relaxed-binding-from-environment-variables):

To convert a property name in the canonical-form to an environment variable name you can follow these rules:

  • Replace dots (.) with underscores (_).
  • Remove any dashes (-).
  • Convert to uppercase.
  • Replace array indexes [x] with x, where x is integer.

-I would say we have what we need. Do you agree @petermicuch?

@petermicuch
Copy link
Contributor Author

petermicuch commented Nov 30, 2020

Hi @jvitrifork. I had to test this now before giving you a statement.

I took your changes, build image and provided these variables:

            - name: HAPI_FHIR_TESTER_ZUMBA_NAME
              value: "DSTU3"
            - name: HAPI_FHIR_TESTER_ZUMBA_SERVER_ADDRESS
              value: "https://host.docker.internal/fhir-dstu3/fhir"
            - name: HAPI_FHIR_TESTER_ZUMBA_REFUSE_TO_FETCH_THIRD_PARTY_URLS
              value: "false"
            - name: HAPI_FHIR_TESTER_ZUMBA_FHIR_VERSION
              value: "DSTU3"

Issue is, that when you specify new key in the map, i.e. in my case ZUMBA, then this is only added to the map next to the other entries coming from application.yaml file. While using array, all entries were overwritten by the environment. Please ignore that SSL issue in the picture and not loaded conformance statement. I am testing with self-signed certificate (would be also nice to have property to accept not secure connections for test purposes).
image

Any idea how else I could get rid of the other two entries other then explicitly overwriting them?

@jvitrifork
Copy link
Collaborator

Well - thats more a question for what kind of defaults that are relevant. If zero defaults is generally preferrable, as it sounds like is your preference, I'll suggest to remove the current defaults in yaml file - @jamesagnew what's your take on that? I have little opinion on this.

@petermicuch
Copy link
Contributor Author

@jvitrifork the idea is mainly, that for small customizations, I do not need to touch application.yaml file at all. I am happy even with current behavior, the only problem is that undescribed behavior of spring boot in case of arrays and usage of underscores _. Otherwise this would not be an issue at all.

@jvitrifork
Copy link
Collaborator

I guess we cant do much about the spring boot behavior - and regarding underscores (see kubernetes/kubernetes#16863), that seems to have a reason on its own.

@jvitrifork
Copy link
Collaborator

@petermicuch Since we are now using named indicies, which makes it easier to reference, what more can be done for now? I agree that the application.yaml should provide as sane defaults as possible,

@petermicuch
Copy link
Contributor Author

@jvitrifork Unless there is a single default in application.yaml, there is no chance to override it with environment, if the same key is not used. To be honest, I would prefer to stay with arrays instead of named indices unless we solve default values dilemma.

I was also not suggesting to use dash in environment name. I was suggesting to switch to dashes instead of underscores in application.yaml. Then it would be clear to omit any dashes in environment variable names as per spring boot documentation. But this seems to be not acceptable for you. So please rather keep the behavior as is without changes, since there is workaround (omit any underscore in environment variable names after usage of array index).

@jvitrifork
Copy link
Collaborator

jvitrifork commented Dec 7, 2020

@petermicuch feel free to make a PR that replaces and transform to your needs. Then we'll have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants