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 v2 backward compatibility #261

Merged
merged 1 commit into from
Sep 6, 2019
Merged

Conversation

jsetton
Copy link
Collaborator

@jsetton jsetton commented Aug 25, 2019

  • restructured project to clearly separate v2/v3

@jsetton
Copy link
Collaborator Author

jsetton commented Aug 25, 2019

@digitaldan I decided to add v2 support back in the skill after reading this documentation which stipulates that the skill should handle both versions until the skill no longer receive any v2 requests.

Also looking at the live skill logs, there are a ton of payload version errors still and these are for actual user requests to control their smart home devices. So rather force these users to spend the time to figure out their issue, we might as well make the experience as smooth as possible 😄

Anyway, let me know what you think. I made a minor project restructuring and added a few minor changes including having rest requests accept gzip compression if available which I think should be enabled with the myopenhab cloud connector #253.

On a side note, the state field is still being pulled in this one. I am hoping that having the request data compressed would resolve that matter.

@jsetton jsetton force-pushed the v2-backward-comp branch 5 times, most recently from 73c62e7 to f390868 Compare August 26, 2019 16:48
@jsetton jsetton changed the title Add skill v2 backward compatibility back in Discovery performance improvement + v2 backward compatibility Aug 26, 2019
@jsetton
Copy link
Collaborator Author

jsetton commented Aug 26, 2019

Ended up finding a way to not use the state field during the discovery process. This means that we can now exclude it from that process 😄 Workaround details are in #257.

@digitaldan hopefully you have some time in the next few days to take a look at this one. I bunched all the changes in one PR. This should cover all the outstanding bug fixes on the skill side.

@jsetton jsetton force-pushed the v2-backward-comp branch 4 times, most recently from c6bfc5d to f1b62ab Compare August 27, 2019 18:23
@jsetton jsetton changed the title Discovery performance improvement + v2 backward compatibility Add v2 backward compatibility Aug 27, 2019
@jsetton
Copy link
Collaborator Author

jsetton commented Aug 27, 2019

So I decided to split the changes in this PR in order to push the discovery performance improvement to live skill while the v2 backward compatibility change can wait for your review. That way, we can see if the discovery change will result in less v2 requests.

@digitaldan
Copy link
Collaborator

Hey Jermey, i have been on vacation for the last week and am getting caught back up with OH stuff today, I will take a look at this and others, apologies for being MIA .

@digitaldan
Copy link
Collaborator

So given the size of the PR i will have to take your word on this one :-) I can't easily tell what code you had to change and what was pulled in from the v2 connecter. I hope it was not as much work as it looks.

@digitaldan
Copy link
Collaborator

Nm my previous comment, i was not looking at the PR correctly and seeing files renamed vs added. Changes looks good to me, super impressed at the v2 test classes you wrote, that does seem like a lot of work! Thanks!

@jsetton
Copy link
Collaborator Author

jsetton commented Aug 28, 2019

I hope it was not as much work as it looks.

Not too much 😄 I had to cleanup and update a bit of the v2 ohConnector to integrate into the current framework. Most of the change is from the folder restructure. I think it makes sense and was needed anyway.

super impressed at the v2 test classes you wrote, that does seem like a lot of work

That part was actually the easiest. We already had the test cases 😄

@jsetton
Copy link
Collaborator Author

jsetton commented Aug 28, 2019

So I just pushed #265 to the live skill and will wait a day before merging and pushing this one to prod. That way we can see if the discovery performance changes improve the number of users that weren't able to have their devices converted to v3 because of the timeout issue.

@digitaldan
Copy link
Collaborator

👍

@digitaldan
Copy link
Collaborator

How did #265 go? Do we need to push this one now?

@jsetton
Copy link
Collaborator Author

jsetton commented Sep 1, 2019

So I just opened #269 related to #265. I think we should resolve that one before merging this PR. Unless you think otherwise.

* restructured project to clearly separate v2/v3

Signed-off-by: jsetton <[email protected]>
@jsetton jsetton merged commit 6c23008 into openhab:master Sep 6, 2019
@jsetton jsetton deleted the v2-backward-comp branch September 6, 2019 13:56
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.

2 participants