opensmarthouse / opensmarthouse-core

Eclipse Public License 2.0
7 stars 0 forks source link

Ensure manually configured folder in env-vars is used #11

Closed cdjackson closed 3 years ago

cdjackson commented 3 years ago

Signed-off-by: Chris Jackson chris@cd-jackson.com

bwosborne2 commented 3 years ago

Is OSH using the openHAB environment variables? I had assumed they would be changed. In other words, OSH_HOME instead ot OPENHAB_HOME

splatch commented 3 years ago

That's fair point @bwosborne2. Keep in mind though that we have to keep some of these due to compatibility reasons. It boils down to any attempt to use System.getProperty(openhab.home) or System.getenv(OPENHAB_HOME) which is beyond our control - this is mainly for third party addons outside of OH.

bwosborne2 commented 3 years ago

OK if I am setting up docker I keep the openHAB environment names? What about the user executing the code? Their user is openhab.

cdjackson commented 3 years ago

It's a fair point. I think we should change the environment variable names to actually avoid a conflict with OH - ie to allow people to have settings for both environments.

bwosborne2 commented 3 years ago

I have a set I started using in my Docker work. Here is a snippet from my Dockerfile. i chose to have the OSH directory in the container off of root like OH does. I removed environments not relevant here. I chose the username smarthouse to try & keep things short.

    GROUP_ID="9001" \
    OSH_BACKUPS="/opensmarthouse/data/backup" \
    OSH_CONF="/opensmarthouse/config" \
    OSH_HOME="/opensmarthouse" \
    OSH_HTTP_PORT="8181" \
    OSH_HTTPS_PORT="8443" \
    OSH_LOGDIR="/opensmarthouse/data/logs" \
    OSH_USERDATA="/opensmarthouse/userdata" \
    OSH_DATA="/opensmarthouse/data" \"
    USER_ID="9001"
bwosborne2 commented 3 years ago

Does that mean my best guess at name changes is OK?

bwosborne2 commented 3 years ago

I assume the compatibility layer then needs to map OSH vars to OH ones for any OH bindings?

cdjackson commented 3 years ago

Which "vars" do you mean? I guess it's not related to this PR though?

splatch commented 3 years ago

I assume the compatibility layer then needs to map OSH vars to OH ones for any OH bindings?

There is a OpenHAB class which can be used to get config folder etc. we could try planting our vars there. WDYT @cdjackson?

cdjackson commented 3 years ago

There is a OpenHAB class which can be used to get config folder etc. we could try planting our vars there.

Again, I'm not super sure what vars you mean?

Are you proposing to change the openhab.userdata type vars to osh.userdata? I'm not against that, but that is not currently part of this PR.

I'm not 100% sure where these parameters are used (other than the OpenHAB class where they support methods to get the user folder etc). In theory, the OpenHAB class should be the only place these are used, so it should be fine - I've just never checked.

Note that the OpenHAB class is just a proxy for the OpenSmartHouse class where the real implementation lies... I've certainly no problem with changing these variable names - it's cleaner in some ways so makes sense - we just need to be sure to do a search to ensure we don't miss any usages I guess.

bwosborne2 commented 3 years ago

I mean things like OH_CONF to OSH_CONF and OH_USERDATAto OSH_USERDATA

cdjackson commented 3 years ago

I mean things like OH_CONF to OSH_CONF and OH_USERDATA to OSH_USERDATA

These are not variables that are used in the code. These are only used in this script file and in the environment variables - the Java code knows nothing about these names so it doesn't need any changes.

bwosborne2 commented 3 years ago

I see the names in the features/karaf/framework/src/main/resources/resources/bin/oh2_dir_layout referenced in this discussion??

cdjackson commented 3 years ago

I see the names in the features/karaf/framework/src/main/resources/resources/bin/oh2_dir_layout referenced in this discussion??

I'm not sure I understand your point here Bruce - that is the file that we are discussing here, so of course if contains these names. This is a script file that reads the environment variables from the OS - it is run before the runtime starts and these names are not used in the runtime itself.

cdjackson commented 3 years ago

I'll merge this. If we want to change the config parameters, which is probably useful, I'd suggest to do that in a separate PR as it's more "complex" in that we need to dig through the code to ensure we get any instances of its use.