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

Make ExecUtil more robust #1700

Merged
merged 5 commits into from
Oct 8, 2020
Merged

Make ExecUtil more robust #1700

merged 5 commits into from
Oct 8, 2020

Conversation

cpmeister
Copy link
Contributor

Ever since I first read this code I've been waiting for an excuse to rewrite it.
I went ahead and got rid of the @@ delimiter usage since it seems silly to not just take the arguments as an additional parameter.

Fixes #1677

Signed-off-by: Connor Petty [email protected]

Make ExecUtil more robust
cd2ce3c 
Signed-off-by: Connor Petty <[email protected]>
@wborn
Copy link
Member

wborn commented Oct 7, 2020

I also never like the usage of @@ and think it is OK to fix (and break) this with OH3.

The org.openhab.core.model.script.actions.Exec class would also need to be updated as scripts will no longer have a way to pass parameters and users will need to adapt their rules for this.

There are also several add-ons that need to be updated for this as well as documentation.

I think it would also be better when the command and parameters always follow each other. That means when specifying a timeout it should be specified as first parameter instead of being in between the command and parameters.

@wborn
Copy link
Member

wborn commented Oct 7, 2020

Being a big fan of java.time I think it would also be more clear to specify the timeout using a Duration so nobody has to look up in docs what the unit is and convert from seconds to milliseconds. 😉

Connor Petty added 2 commits October 7, 2020 09:53
Adjusted according to code review
a5739fa 
Signed-off-by: Connor Petty <[email protected]>
Fix script Exec class.
f467089 
Signed-off-by: Connor Petty <[email protected]>
@cpmeister
Copy link
Contributor Author

cpmeister commented Oct 7, 2020 •
edited
Loading

I've gone ahead and made the recommended changes, the only reason I didn't do it the first time is because I didn't want to break existing code. But if you are ok breaking some code then I'd be more than happy to.

From a quick search it looks like only 4 addons are using the ExecUtil right now so fixing those will be very easy.

wborn
wborn reviewed Oct 7, 2020
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Great! Can you also create PR for the add-ons so we can limit the time builds are broken?

bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java Outdated Show resolved Hide resolved
bundles/org.openhab.core.io.net/src/main/java/org/openhab/core/io/net/exec/ExecUtil.java Outdated Show resolved Hide resolved
@paulianttila
Copy link
Contributor

I also never like the usage of @@

Me either, but as an excuse, it was selected to be exec1 binding syntax compatible ;)

https://github.com/openhab/openhab1-addons/tree/master/bundles/binding/org.openhab.binding.exec

Fixed javadoc
c7a47bc 
Signed-off-by: Connor Petty <[email protected]>
@cpmeister
Copy link
Contributor Author

Great! Can you also create PR for the add-ons so we can limit the time builds are broken?

Done: openhab/openhab-addons#8690

Fix javadoc
38168e3 
Signed-off-by: Connor Petty <[email protected]>
wborn
wborn approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks!

@wborn wborn merged commit 0dfda1e into openhab:master Oct 8, 2020
@wborn wborn added this to the 3.0 milestone Oct 10, 2020
@kaikreuzer kaikreuzer modified the milestones: 3.0, 3.0.0.M1 Oct 11, 2020
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature of the Core label Oct 11, 2020
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rule-issue/106732/11

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-0-milestone-2-discussion/107564/97

Confectrician added a commit to Confectrician/openhab-docs that referenced this pull request Dec 22, 2020
@Confectrician
Exec actions timeout is now in seconds. Reference: openhab/openhab-co…
44787be 
…re#1700

Signed-off-by: Jerome Luckenbach <[email protected]>
@Confectrician Confectrician mentioned this pull request Dec 22, 2020
Merged
Confectrician added a commit to openhab/openhab-docs that referenced this pull request Dec 22, 2020
@Confectrician
Exec actions timeout is now in seconds. (#1360)
0b7f7c9 
* Exec actions timeout is now in seconds. Reference: openhab/openhab-core#1700

Signed-off-by: Jerome Luckenbach <[email protected]>

* Details on how to use Duration now.

Signed-off-by: Jerome Luckenbach <[email protected]>
@holgerfriedrich holgerfriedrich mentioned this pull request Dec 22, 2020
Closed
@kaikreuzer kaikreuzer mentioned this pull request Jan 11, 2021
Closed
@NorbertHD NorbertHD mentioned this pull request Jan 14, 2021
Merged
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
@cpmeister @splatch
Make ExecUtil more robust (openhab#1700)
c3b89b1 
Signed-off-by: Connor Petty <[email protected]>
GitOrigin-RevId: 0dfda1e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
3.0.0.M1
Development

Successfully merging this pull request may close these issues.

ExecUtil.executeCommandLine() is broken
5 participants
@cpmeister @wborn @paulianttila @openhab-bot @kaikreuzer