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

Adding new config option that disables registration #57

Merged
merged 2 commits into from Apr 5, 2017

Conversation

ryanrdetzel
Copy link
Contributor

If you're running your own instance you probably don't want others registering.

Small bug fix where the link was hardcoded to https://home.myopenhab.org.

@marziman
Copy link
Contributor

hi @ryanrdetzel ,

I am reviewing this and give feedback this days. Thanks!

@kobelka
Copy link

kobelka commented Mar 17, 2017

Works for me

Copy link
Contributor

@marziman marziman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanrdetzel I ve reviewed this PR and just recognized that you had an "older" code base. We ve removed the "bcrypt-nodejs" in the package.json and therefor its reference in the code needs to be out, too. *Should be no big deal and can be merged..

Copy link
Contributor

@marziman marziman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanrdetzel you ve removed the home.myopenhab link which is not a bug actually. This works on your machine/instance now for sure, but will break the link for the myopenhab.org machine of the foundation.
When I just tested it locally on my workstation it also didn't work properly without changing the port from :3000 to :8080.
My Proposal is: Either we take this out at the moment or we make the "home-dashboard" proxying URL configurable. But if you want you can take this out..

@digitaldan
Copy link
Contributor

digitaldan commented Mar 24, 2017

I ve reviewed this PR and just recognized that you had an "older" code base. We ve removed the "bcrypt-nodejs" in the package.json and therefor its reference in the code needs to be out, too.

@marziman I'm not sure I understand, his changes are against an earlier revision, but that's ok, thats how revision control works, git will merge the changes unless there are conflicts. Did I miss something else?

@digitaldan
Copy link
Contributor

you ve removed the home.myopenhab link which is not a bug actually. This works on your machine/instance now for sure, but will break the link for the myopenhab.org machine of the foundation.

Just for clarification, we prefer to use a "full proxy" host name, like home.myopenhab.org that does not require complex routes that try and match every possible URL on a OH instance, its very hard to keep this up to date, and in fact we probably don't have all the mappings even now. I would love to git rid of this, but there are too many legacy apps that use it. By using a host name different then the main server we can blindly proxy the request to an OH server.

@marziman
Copy link
Contributor

@digitaldan yes I know. This shouldn't be a problem and we can merge it. No the only thing was "bcrypt-nodejs", but this will be no problem for git :-)
I will wait for his feedback regarding the URL and will merge it in.

@digitaldan
Copy link
Contributor

Hi @ryanrdetzel we have another PR that also tries to change the home URL, can you update your PR to make this configurable or at least take it out so we can do this in another branch? Thanks

Copy link
Contributor Author

@ryanrdetzel ryanrdetzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@marziman
Copy link
Contributor

marziman commented Apr 5, 2017

Tested and going to merge in

@marziman marziman merged commit 81c258f into openhab:master Apr 5, 2017
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.

None yet

4 participants