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.

Sign up for GitHub

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

Jump to bottom

[amazonechocontrol] Partial revert of PR 7919 #8074

Merged
merged 1 commit into from
Jul 23, 2020
Merged

[amazonechocontrol] Partial revert of PR 7919 #8074

merged 1 commit into from
Jul 23, 2020

Conversation

lolodomo
Copy link
Contributor

@lolodomo lolodomo commented Jul 4, 2020

Related to #8027

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo
[amazonechocontrol] Partial revert of PR 7919
44c0855 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo requested a review from mgeramb as a code owner July 4, 2020 15:14
@lolodomo lolodomo mentioned this pull request Jul 4, 2020
Closed
12 tasks
@TravisBuddy
Copy link

Travis tests have failed

Hey @lolodomo,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

J-N-K
J-N-K previously requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Has this been properly tested? I would want to avoid introducing regressions.

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 7, 2020

No, I can't test.
I proposed several PRs related to the issue just to help, even on bindings I am not using at all.

@kaikreuzer
Copy link
Member

@J-N-K What concerns do you have wrt this change that you flag it as "changes requested"?
It seems to be a logical fix to me and if we'd get an lgtm from @mgeramb, I'd be ok to merge it.

@J-N-K J-N-K dismissed their stale review July 11, 2020 15:00

If merging untested changes to working code is ok, feel free to merge.

@J-N-K
Copy link
Member

J-N-K commented Jul 11, 2020

My concern is: this code was broken before and crashed the whole openHAB instance. It was fixed and confirmed working. Now a change is introduced in that part of the code without proper testing just to make it look better.

@kaikreuzer
Copy link
Member

just to make it look better.

Well, I think this is not the intention here.
This PR originates from @sprehn's input from #8027 (comment), which is imho a valid concern about the current code, which this PR addresses.

@lolodomo
Copy link
Contributor Author

What are we doing here ? @J-N-K : do you want to test this change yourself before its merge ?

@J-N-K
Copy link
Member

J-N-K commented Jul 19, 2020

No, I don't want to test it and I'll not merge it, But if someone wants to do it, I'm ok with it.

@kaikreuzer
Copy link
Member

I am fine to merge it, but I would wait with it until the 2.5.7 release is done. We will then have a few weeks where people can test and complain, if there is any problem, before the PR goes into 2.5.8.

@kaikreuzer kaikreuzer merged commit c16438f into openhab:2.5.x Jul 23, 2020
@lolodomo lolodomo deleted the amazonecho_wsclientstop branch July 23, 2020 16:51
CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Jul 26, 2020
@lolodomo @CSchlipp
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
77f72b8 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: CSchlipp <christian@schlipp.de>
MPH80 pushed a commit to MPH80/openhab-addons that referenced this pull request Aug 3, 2020
@lolodomo @MPH80
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
b969186 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: MPH80 <michael@hazelden.me>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
730442d 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
c74f9aa 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
1032604 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020
@lolodomo @andrewfg
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
8818469 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020
@lolodomo @DaanMeijer
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
b79e056 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Signed-off-by: Daan Meijer <daan@studioseptember.nl>
markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020
@lolodomo @markus7017
[amazonechocontrol] Partial revert of PR 7919 (openhab#8074)
2d04bf3 
Related to openhab#8027

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@J-N-K J-N-K J-N-K left review comments

@mgeramb mgeramb Awaiting requested review from mgeramb mgeramb is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lolodomo @TravisBuddy @kaikreuzer @J-N-K