Skip to content

Conversation

@bostko
Copy link
Contributor

@bostko bostko commented Oct 9, 2015

@alasdairhodge , @aledsage Can you review it.
I think it is good to have such methods exposed.

I would even suggest to remove the compileScript but external projects are still dependent on it.

Copy link
Member

Choose a reason for hiding this comment

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

Should we just overload the executePs method (with this one taking a string), rather than using a different name?

Same for executeCommand: I'd prefer consistency of naming. If we think that executeScript was not the best name, then we can look at changing that name.

Copy link
Member

Choose a reason for hiding this comment

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

We should include some javadoc to say what this command does (it's unfortunate that there wasn't any javadoc on the original executePs(List) method).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aledsage
What do you thing about renaming it only for Ps, executePs(String) and executePs(List<String>).
and keep executeCommand(String) and executeCommand(List) for the "Command Prompt" commands? Really we do notexecuteScript` no script was being composed here so I think it is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, so +1. But let's keep old names as deprecated until Brooklyn catches up.

@bostko
Copy link
Contributor Author

bostko commented Oct 12, 2015

@aledsage can you review it again

@bostko bostko force-pushed the single_command branch 2 times, most recently from 625df2b to 8f8f899 Compare October 12, 2015 15:24
Copy link
Member

Choose a reason for hiding this comment

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

This kind of thing is implementation details rather than part of the contract. I'd therefore be very hesitant about including it in the javadoc.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the reason for the method renames (rather than just overloading the existing methods that have the same behaviour - e.g. executeScript(List) is exactly like this executeCommand(String) if you pass a list of size one).

@bostko
Copy link
Contributor Author

bostko commented Jan 11, 2016

Notice it depends on apache/incubator-brooklyn#950

@bostko bostko force-pushed the single_command branch 8 times, most recently from a90b9c4 to fb23919 Compare January 15, 2016 14:54
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I deprecated it. @neykov has several tests for executePs(List<String>)

@bostko
Copy link
Contributor Author

bostko commented Jan 15, 2016

@neykov can you review it

@neykov
Copy link
Contributor

neykov commented Jan 15, 2016

@bostko I think we should keep the (List<String>) methods and just add (String) overloads (just for sugaring over the list alternative). What's the reason for removing them?
Note that the winrm service doesn't support concatenated lines with \r\n nicely - it will return the output of the first command only (see disabled tests). That means that the executeCommand(String) is really just for a single line of batch script.
Another reason for keeping them is that we can optimize and open a single shell, then execute each command from the list, then close the shell (instead of opening a shell for each command as is currently which will probably break scripts depending on environment changes).

@bostko bostko force-pushed the single_command branch 3 times, most recently from 57b1dc7 to 29a5a90 Compare January 15, 2016 20:09
@bostko
Copy link
Contributor Author

bostko commented Jan 16, 2016

@neykov @aledsage

  • I kept the old signatures.
  • executeScript(List) is chaining commands with & now instead of "\r\n"

Copy link
Contributor

Choose a reason for hiding this comment

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

Wht not create analogous assertExecPs, instead of making compilePs public?

Copy link
Member

Choose a reason for hiding this comment

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

+1 will change that in my branch of this.

- moved to a working version of jaxws-maven-plugin
- exit codes assertion
@aledsage
Copy link
Member

Closing - I've merged #11 which includes this commit.

@aledsage aledsage closed this Jan 18, 2016
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.

3 participants