OHDSI / WebAPI

OHDSI WebAPI contains all OHDSI services that can be called from OHDSI applications
Apache License 2.0
125 stars 165 forks source link

Application properties management #1125

Open pavgra opened 5 years ago

pavgra commented 5 years ago

Today application properties are managed via pom.xml and the application.properties file works as a "proxy". I'd like to figure out what was the reasoning for the approach and suggest to migrate app configuration to application.properties and keep only dependencies in pom.xml.

@anthonysena , @fdefalco , @chrisknoll

chrisknoll commented 5 years ago

Discussed here: http://forums.ohdsi.org/t/webapi-properties/219

I would prefer an approach that allows specifying application configuration settings independent of a build step. In my experence, keeping the configuration separate from build means you are less likely to deploy QA settings into a Prod env or worse a PROD environment into a DEV env.

pavgra commented 5 years ago

I would prefer an approach that allows specifying application configuration settings independent of a build step

Let me confirm: so you are in support of extracting configuration properties from pom.xml and moving them into application.properties (application.yml), right?

chrisknoll commented 5 years ago

I'm not clear on the tradeoffs of one approach vs. another. I'm just indicating that I would prefer that the function of our application configuration worked as: each deployment environment has a set of configuration values that when the application starts up in that environment, it reads the application configuration local to that environment. In this way, we do not bind the build to the environment, which in my experience leads to wrong configuration being deployed (usually leading to catastrophic results).

If you know of different options than what we use today (which currently is: pom.xml properties are copied out to application.properties at build time, and application properties are read in by Spring to populate @value references), I'm sure we would all be happy to hear about what those other options are, and discuss the pros and cons of each approach.

pavgra commented 5 years ago

In this way, we do not bind the build to the environment

But neither you do today. Spring already reads the application configuration local to that environment. Or what I got wrong?

If you know of different options than what we use today

I know that:

chrisknoll commented 5 years ago

But neither you do today. Spring already reads the application configuration local to that environment. Or what I got wrong?

Nothing. You're correct that spring reads app config. But, what you're leaving out is how those values are set. Our WebAPI Install Guide has an entire section on configuring the maven profile. This profile is not only controlling dependencies but setting up security, JDBC urls, etc. So, while you're 100% correct on the spring config, the missing points are how we currently configure application settings today, what our documentation instructs end users to do, and how we would like that to change in the future.

My only point was that if we move off of our current approach (which invovles using a settings.xml file and a maven build to overwrite values in application.properties) was that it would be nice to be able to have a compiled .WAR file be able to just read an application configuration at startup so that deploying WebAPI to dev/stage/prod can just be a single .WAR deploy. Today the process is build war with dev settings -> deploy to dev env, build war with stage settings -> deploy war to stagve env, build war with prod settings -> deploy to prod env. The risk is accidently this happenign build file wtih prod settings -> deploy war to dev env.

pavgra commented 5 years ago

it would be nice to be able to have a compiled .WAR file be able to just read an application configuration at startup so that deploying WebAPI to dev/stage/prod can just be a single .WAR deploy. Today the process is build war with dev settings -> deploy to dev env, build war with stage settings -> deploy war to stagve env, build war with prod settings -> deploy to prod env.

And this is where I actually disagreed. The statement is wrong: you don't need to re-build WAR to run it with new properties. You can easily provide new properties via command line or environment variables on stratup of already built WAR and such properties would override props defined in application.properties file (which were set at the moment of build). Documentation: https://docs.spring.io/spring-boot/docs/current/reference/html/boot-features-external-config.html

So, you see, this is the example of why I propose to change the approach: even you got confused by this mixture of pom.xml and application.properties responsibilities.

chrisknoll commented 5 years ago

Pavel, there's truly no confusion here. I'm patiently trying to explain to you the origins of the process (which is what your original question asked). The answer is, according to the old OHSI post from 2015, that they wanted to populate it via maven plugins. So, that's what they did.

You say we don't need to do that and we can override spring's application props via command line? Great, sounds like it removes the dependency on running a maven command to inject values into the application.properties file, which means you don't need to make a special mvn build. Wonderful. All we need is updated instructions about how to launch the WAR in something like tomcat with those environment specific values. Sounds like you'd like to go further and remove other build directives from pom.xml to make things cleaner, go ahead.

I've never really dug too deeply into the actual mechanism that translates the <property> elements in pom.xml into the application.properties file. Others made the decision to do it that way, and I followed the model like a good collaborator. We can always revisit those decisions, and since there's advantages to doing it via spring external configuration, we should pursue it.

If @cahilton or @fdefalco would like to put in their thoughts to moving to the new approach or justify the original implementation, they should make it known here.

-Chris

pavgra commented 5 years ago

Sounds like you'd like to go further and remove other build directives from pom.xml to make things cleaner, go ahead

Yes, I'd like to. And trying to be a good collaborator I posted the thoughts first to have a discussion and make everyone aware. Then, I think, we'll propose a PR to address this item

anthonysena commented 5 years ago

From our discussion today on the Atlas/WebAPI WG call, the idea of having pom.xml focused on dependency management and application.properties as the application configuration is a desired state.

The additional action that we also discussed is that the WebAPI documentation will need to reflect the new build/deployment process that accompanies these changes.