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

Add a new system module which holds system configuration #81

Merged
merged 2 commits into from
Apr 23, 2017

Conversation

FlorianSW
Copy link
Member

Currently only the host, port and protocol information, which are accessible
through the provided getter functions, but it can provide access to other
configurations and settings, too.

This improves the handling of the host, port and protocol part of a base url,
which was currently handled by a single configuration variable, only. With this
it was unclear, what the configuration holds and what is expected (with protocol,
or without, with port or without, and so on). The configuration also slightly
changed. The system object know provides a separate way for specifying the base
url with the new options:

  • protocol: The protocol of the "public" accessible URL for this service (e.g. http
    or https)
  • host: The host- or domain name from which this service is accesible, e.g. localhost
    or example.com
  • port: The port over which this service is accesible, e.g. 80 or 443.

This also fixes #45 by using the new getBaseURL() method of the System class to retrieve
the base url of the service and appends the path to that, instead of a hard-coded URL.

@digitaldan digitaldan self-requested a review April 20, 2017 23:23
@digitaldan digitaldan self-assigned this Apr 20, 2017
@digitaldan
Copy link
Contributor

Very cool btw, much better than passing around the config json, reviewing now.

@digitaldan
Copy link
Contributor

@marziman FYi this will break current configurations until they update their config, which I'm ok with, just something to be aware of.

@FlorianSW
Copy link
Member Author

Interesting, I used github's conflict resolution teh first time :P If the separate merge commit isn't ok (which I would totally understand), I'll squash both commits, just leave a note :)

@digitaldan Hmm, it shouldn't, I tried to implement an on-the-fly migration in the constructor of System to not break current configurations. Do I miss something, or have I implemented it wrong? :)

@digitaldan
Copy link
Contributor

i mean anyone who is implementing their own cloud install will need to add host/port/protocol to their config after this PR is committed

@FlorianSW
Copy link
Member Author

Oh, yes, you're right, however, I'm totally fine with it. From what I understand, the configuration options in the base section are required system settings. I would expect the system start to fail if the settings aren't set :) But thanks for the note, I hope that's ok for all users.

@digitaldan
Copy link
Contributor

Sorry, I see you do support backwards compatibility here https://github.com/openhab/openhab-cloud/pull/81/files#diff-5c350865b90e8c9c7f2b316f9c84f74fR17 , but when I tried it with an existing configuration the links in my emails looked like this "mydomain.com://443:null/lostpasswordreset?resetCode=51535fe0-456d-11e7-8d3b-ab18cbf0bfa1"

@FlorianSW
Copy link
Member Author

Oh damn... I assume you have the following in your baseurl setting:
"mydomain.com:443"
right? This will obviously fail, because the protocol is missing. Let me check, if we can workarount this :)

Currently only the host, port and protocol information, which are accessible
through the provided getter functions, but it can provide access to other
configurations and settings, too.

This improves the handling of the host, port and protocol part of a base url,
which was currently handled by a single configuration variable, only. With this
it was unclear, what the configuration holds and what is expected (with protocol,
or without, with port or without, and so on). The configuration also slightly
changed. The system object know provides a separate way for specifying the base
url with the new options:
 - protocol: The protocol of the "public" accessible URL for this service (e.g. http
   or https)
 - host: The host- or domain name from which this service is accesible, e.g. localhost
   or example.com
 - port: The port over which this service is accesible, e.g. 80 or 443.

This also fixes openhab#45 by using the new getBaseURL() method of the System class to retrieve
the base url of the service and appends the path to that, instead of a hard-coded URL.
@FlorianSW
Copy link
Member Author

@digitaldan I accidently[1] used git commit --amend and was too lazy to split the commit manually, so I force pushed. The change happened in 3e4a3a5#diff-5c350865b90e8c9c7f2b316f9c84f74fR19 and the lines after. I hope that's ok :)

[1] In another project where I'm mostly active, we're using gerrit, where a change is updated by amending the commit, and my fingers were faster as my head :)

@marziman
Copy link
Contributor

@digitaldan:
Thanks. I think it is ok to improve the config by this way. We might add this to docu.
@FlorianSW: Yes protocol was not there before, since it was URL:PORT

@digitaldan
Copy link
Contributor

LGTM

@digitaldan digitaldan merged commit 11a8504 into openhab:master Apr 23, 2017
@FlorianSW
Copy link
Member Author

Thanks! :)

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

Successfully merging this pull request may close these issues.

absolute url in openhab-cloud/views/index.ejs
3 participants