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 new regional settings service pid support #341

Closed
jsetton opened this issue Dec 20, 2019 · 8 comments · Fixed by #344
Closed

Add new regional settings service pid support #341

jsetton opened this issue Dec 20, 2019 · 8 comments · Fixed by #344
Labels

Comments

@jsetton
Copy link
Collaborator

jsetton commented Dec 20, 2019

The regional settings service pid changed in openHAB 2.5 from org.eclipse.smarthome.core.i18nprovider to org.eclipse.smarthome.i18n

openhab/openhab-core#1112

@jsetton jsetton added the bug label Dec 20, 2019
@jsetton
Copy link
Collaborator Author

jsetton commented Dec 20, 2019

@digitaldan since we need to have backward compatibility, do you have any idea how we can support this without doing two api calls?

@jsetton
Copy link
Collaborator Author

jsetton commented Dec 21, 2019

@digitaldan I just pushed a PR for this one. The discovery process has now an extra service config api call for the new pid support. It's an async call so it shouldn't affect much the overall execution since the get all items call is most likely the longest to complete. I am going to merge it for now and release to live skill. If you feel, there is a better solution to resolve this one, let me know.

@oeiber
Copy link

oeiber commented Dec 22, 2019

@digitaldan my reverse proxy is configured to allow only the necessary api calls from aws lamda to the backend. that means during discovery first a api call to the old url is done and my nginx replies a 403.
Is it possible to do the api call to the new url first and if it fails do a call to the old url?
thx!

@jsetton
Copy link
Collaborator Author

jsetton commented Dec 22, 2019

@oeiber It would take additional execution time, on every discovery requests, to do two consecutive calls compared to the current asynchronous implementation. Our main goal is to keep that time as low as possible from AWS resources cost perspective. Out of curiosity, is there any reason why you can't allow both urls?

@oeiber
Copy link

oeiber commented Dec 22, 2019

@jsetton ok. i understand. sure its possible to allow both urls. but from your perspective wouldn‘t it be more useful to add oh‘s version to config.js to avoid the 2nd api call?

@jsetton
Copy link
Collaborator Author

jsetton commented Dec 22, 2019

but from your perspective wouldn‘t it be more useful to add oh‘s version to config.js to avoid the 2nd api call?

While it might work for the private setup, we can't get that information ahead of time from the users of the official skill.

@digitaldan
Copy link
Collaborator

@jsetton sorry i missed this, sounds good.

@jsetton
Copy link
Collaborator Author

jsetton commented Oct 10, 2020

@oeiber with the change part of #366, your wish has actually been granted! 😄

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

Successfully merging a pull request may close this issue.

3 participants