You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2015/08/25 15:06:54 UTC
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
GitHub user bostko opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/866
Winrm commands stdin, stdout, stderr; Commands Live tests
This PR provides stdin, stderr and stdout support to the Windows commands.
Look the commits to see the changes
@nakomis Can you please review this?
It has a Live tests for it and there is an example blueprint for windows byon location which I can remove from the PR if you'd like.
I also changed the config key ByonLocationResolver.OS_FAMILY. Here are my comments on these
So JcloudsLocationConfig.OS_FAMILY config key is osFamily and for ByonLocationResolver.OS_FAMILY it is osfamily without capital F
I was thinking for fixing it by moving this config key to AbstractLocation. Is it possible that we don't want to include this config to all descendants of AbstractLocation?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/incubator-brooklyn winrm_std
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/866.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #866
----
commit 48fef8a18aff07ae8caeea2d8a89493da32bdbba
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-19T21:05:23Z
Add stdOut and stdErr to WinRmDriver
commit 348a8f74695faa743458b65f1b47e5fa8ee9758a
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-20T12:41:47Z
findTaskOrSubTask lower the logging level to debug
commit db7a63e52151f9eb689903edcbc97d89f4e518a0
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-20T21:33:44Z
Commit not working VanillaSoftwareProcessWinrmStreamsLiveTest
commit b6836d1cb011fc9f999e17c3e02ef1b31c563503
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-21T16:18:20Z
Rename ByonLocation's attribute osfamily to osFamily
commit 2ae58dd27502de053cf0d6fe50703909f9522a5c
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-24T14:24:13Z
Added Windows process streams test
- refactored streams tests
commit f748bec7d182d9bea6fd71cc04ab712140cd954c
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-08-25T12:10:05Z
Power Shell Commands test
- refactored runPreInstallCommand
- fix imports
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-135379302
@bostko looks good - a few more minor comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966835
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/VanillaSoftwareProcessStreamsIntegrationTest.java ---
@@ -59,8 +39,24 @@ public void setUp() throws Exception {
}
@Test(groups = "Integration")
+ @Override
public void testGetsStreams() {
- Map<String, String> cmds = ImmutableMap.<String, String>builder()
--- End diff --
Slightly simpler to do `Map<String, String> cmds = getCommands();`. And then below to use `cmds` rather than repeatedly calling `getCommands()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38081786
--- Diff: core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverTest.java ---
@@ -345,7 +345,7 @@ public void testWindowsMachines(String osFamily) throws Exception {
String spec = "byon";
Map<String, ?> flags = ImmutableMap.of(
"hosts", ImmutableList.of("1.1.1.1", "2.2.2.2"),
- "osfamily", osFamily
+ "osFamily", "windows"
--- End diff --
Please use the `osFamily` parameter that is being passed in - i.e. `"osFamily", osFamily`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38178746
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -119,20 +136,53 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
String regularCommand = getEntity().getConfig(regularCommandKey);
String powershellCommand = getEntity().getConfig(powershellCommandKey);
- if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
+ if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
if (allowNoOp) {
return new WinRmToolResponse("", "", 0);
} else {
throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
+ } else if (!Strings.isBlank(regularCommand) && !Strings.isBlank(powershellCommand)) {
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ ByteArrayOutputStream stdIn = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
+
+ boolean tagWithStd = Tasks.current() != null;
+ if (tagWithStd) {
+ writeToStream(stdIn, Strings.isBlank(regularCommand) ? powershellCommand : regularCommand);
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdIn));
+
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdOut));
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDERR, stdErr));
+ }
+ }
+
+ WinRmToolResponse response;
+ if (Strings.isBlank(regularCommand)) {
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (tagWithStd) {
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
--- End diff --
But the way you've followed that is different. In https://github.com/apache/incubator-brooklyn/blob/master/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java#L262 it has the `if` guard for the call to `Tasks.addTagDynamically`. You have done that a few lines above.
My point here is that once you've called `Tasks.addTagDyamically` a few lines above, you don't want to then do the `if` guard again for when you call `writeToStream` - I'd expect that second if to return false because you previously added the steam tag for this task.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-134939252
Thanks @bostko - finished reviewing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966433
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -129,10 +132,40 @@ protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey,
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
+ WinRmToolResponse response;
if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (Tasks.current()!=null) {
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDIN)==null) {
+ if (Strings.isNullOrEmpty(regularCommand)) {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDIN, powershellCommand);
+ } else {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDIN, regularCommand);
+ }
+ }
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDOUT, response.getStdOut());
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDERR, response.getStdErr());
+ }
+ }
+
+ return response;
+ }
+
+ private void tagDynamicallyWithSoftStream(String brooklynTaskTag, String std) {
+ try {
+ ByteArrayOutputStream stdStream = new ByteArrayOutputStream();
+ stdStream.write(std.getBytes());
+
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(brooklynTaskTag, stdStream));
+
+ if(LOG.isDebugEnabled()) LOG.debug("{} {}", brooklynTaskTag, std);
+ } catch (IOException e) {
+ LOG.warn(Joiner.on('\n').join(e.getStackTrace()));
--- End diff --
I'd log with `LOG.warn("Problem populating "+brooklynTaskTag+" for task of entity "+getEntity(), e);`
It's better to let the logging framework know it's an exception, and to give more context for what was being done when the exception was thrown.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966626
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java ---
@@ -37,31 +42,36 @@ public void start() {
super.start();
}
-
+
@Override
- public void preInstall() {
- super.preInstall();
- executeCommand(VanillaWindowsProcess.PRE_INSTALL_COMMAND, VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND, true);
- if (entity.getConfig(VanillaWindowsProcess.PRE_INSTALL_REBOOT_REQUIRED)) {
- rebootAndWait();
+ public void runPreInstallCommand() {
+ if(Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.PRE_INSTALL_COMMAND)) || Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND))) {
+ executeCommand(VanillaWindowsProcess.PRE_INSTALL_COMMAND, VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND, true);
+ if (entity.getConfig(VanillaWindowsProcess.PRE_INSTALL_REBOOT_REQUIRED)) {
--- End diff --
If one had configured `PRE_INSTALL_REBOOT_REQUIRED` without any pre install command, we should still execute it. Let's move this check outside of the outer if block.
Also, I'd use `if (Boolean.TRUE.equals(entity.getConfig(VanillaWindowsProcess.PRE_INSTALL_REBOOT_REQUIRED))) {`. Otherwise there is a risk of a NullPointerException if the config parameter's value is null.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37965596
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -129,10 +132,40 @@ protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey,
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
+ WinRmToolResponse response;
if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (Tasks.current()!=null) {
--- End diff --
I'd be inclined to add the `tagDynamicallyWithSoftStream` before executing the script. Then after the script has executed, update that stream with the contents of `response.getStdOut()` etc.
If you did that, then while the command is executing you'd be able to inspect the stdin of the command and you'd be able to see that there will be a stdout/stderr (but they would be empty). The alternative here would be surprising for users (i.e. in the activity view, it does not show that there will be any streams; then when you navigate away from the task and back again once it's finished it shows that there are now streams).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37964775
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java ---
@@ -116,11 +116,9 @@ public void start() {
preInstall();
}});
- if (Strings.isNonBlank(entity.getConfig(BrooklynConfigKeys.PRE_INSTALL_COMMAND))) {
- DynamicTasks.queue("pre-install-command", new Runnable() { public void run() {
- runPreInstallCommand(entity.getConfig(BrooklynConfigKeys.PRE_INSTALL_COMMAND));
- }});
- };
+ DynamicTasks.queue("pre-install-command", new Runnable() { public void run() {
--- End diff --
Seems wrong to have "pre-install-command" as a special case. There is still a check that the command is non-blank for `POST_INSTALL_COMMAND`, `PRE_LAUNCH_COMMAND` and `POST_LAUNCH_COMMAND`. We should either include the if check for all of them or for none of them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-134934816
I will appreciate if anyone review it earlier.
Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37964411
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -199,8 +199,8 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
}
Class<? extends MachineLocation> locationClassHere = locationClass;
- if (osfamily != null) {
- locationClassHere = getLocationClass(osfamily);
+ if (osFamily != null) {
+ locationClassHere = OS_TO_MACHINE_LOCATION_TYPE.get(osFamily);
--- End diff --
This reverts @sjcorbett 's change in commit c3e027b80826427d04e3b726bf9f71f24fc46103. Can you change it back to use `getLocationClass(osFamily)` please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966051
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -129,10 +132,40 @@ protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey,
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
+ WinRmToolResponse response;
if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (Tasks.current()!=null) {
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDIN)==null) {
+ if (Strings.isNullOrEmpty(regularCommand)) {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDIN, powershellCommand);
+ } else {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDIN, regularCommand);
+ }
+ }
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDOUT, response.getStdOut());
+ tagDynamicallyWithSoftStream(BrooklynTaskTags.STREAM_STDERR, response.getStdErr());
+ }
+ }
+
+ return response;
+ }
+
+ private void tagDynamicallyWithSoftStream(String brooklynTaskTag, String std) {
+ try {
+ ByteArrayOutputStream stdStream = new ByteArrayOutputStream();
+ stdStream.write(std.getBytes());
+
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(brooklynTaskTag, stdStream));
+
+ if(LOG.isDebugEnabled()) LOG.debug("{} {}", brooklynTaskTag, std);
--- End diff --
To double-check: is this the only place the stdout / stderr is being logged? Or will we now have multiple copies of stdout being logged? We only want one copy in the log.
It's also definitely worth including more context in the log message. If it just says "stdout ...." then you can't tell which entity it was for or which machine it was on. Therefore include a prefix. For example, here is a log statement for ssh stdout (which is written one line at a time):
2015-08-18 12:58:13,400 DEBUG brooklyn.SSH [Thread-50601]: [amp@<ip>:22:stdout] WARNING: no known/successful package manager to install iptables-persistent, may fail subsequently
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38088184
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -119,20 +136,53 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
String regularCommand = getEntity().getConfig(regularCommandKey);
String powershellCommand = getEntity().getConfig(powershellCommandKey);
- if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
+ if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
if (allowNoOp) {
return new WinRmToolResponse("", "", 0);
} else {
throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
+ } else if (!Strings.isBlank(regularCommand) && !Strings.isBlank(powershellCommand)) {
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ ByteArrayOutputStream stdIn = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
+
+ boolean tagWithStd = Tasks.current() != null;
+ if (tagWithStd) {
+ writeToStream(stdIn, Strings.isBlank(regularCommand) ? powershellCommand : regularCommand);
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdIn));
+
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdOut));
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDERR, stdErr));
+ }
+ }
+
+ WinRmToolResponse response;
+ if (Strings.isBlank(regularCommand)) {
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (tagWithStd) {
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
--- End diff --
I don't think you want this additional `if`. I'm surprised it works with it. We've added a stream to this task previously, so when you check if the current task has a null stdout stream, I'd expect it to return false.
Also, you're just writing to your stdout / stderr streams. If someone else has replaced the tag for some reason, then you writing to your streams won't affect that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38082968
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java ---
@@ -72,8 +72,12 @@ public void launch() {
@Override
public boolean isRunning() {
- return executeCommand(VanillaWindowsProcess.CHECK_RUNNING_COMMAND,
- VanillaWindowsProcess.CHECK_RUNNING_POWERSHELL_COMMAND, false).getStatusCode() == 0;
+ WinRmToolResponse runningCheck = executeCommand(VanillaWindowsProcess.CHECK_RUNNING_COMMAND,
+ VanillaWindowsProcess.CHECK_RUNNING_POWERSHELL_COMMAND, false);
+ if(runningCheck.getStatusCode() != 0) {
+ LOG.info(this + " isRunning check failed: exit code " + runningCheck.getStatusCode() + "; " + runningCheck.getStdErr());
--- End diff --
Instead of logging `this`, I'd log the `getEntity()` (the toString on the driver might not be good).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-135668735
I'll merge this now, and then change the line in `AbstractSoftwareProcessWinRmDriver` as discussed in the comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37964535
--- Diff: core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverTest.java ---
@@ -345,7 +345,7 @@ public void testWindowsMachines(String osFamily) throws Exception {
String spec = "byon";
Map<String, ?> flags = ImmutableMap.of(
"hosts", ImmutableList.of("1.1.1.1", "2.2.2.2"),
- "osfamily", osFamily
+ "osFamily", "windows"
--- End diff --
This reverts Sam's change (where he tested it is case insensitive). Can you revert to use the `osFamily` parameter please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966653
--- Diff: core/src/test/java/org/apache/brooklyn/location/byon/ByonLocationResolverTest.java ---
@@ -345,7 +345,7 @@ public void testWindowsMachines(String osFamily) throws Exception {
String spec = "byon";
Map<String, ?> flags = ImmutableMap.of(
"hosts", ImmutableList.of("1.1.1.1", "2.2.2.2"),
- "osfamily", osFamily
+ "osFamily", "windows"
--- End diff --
I am doing exactly this.
I've changed it from osfamily to osFamily.
Notice that this was only in ByonLocationResolver. Can you check my PR comment on top. What do you think for moving this parameter in AbstractLocation
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966662
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java ---
@@ -37,31 +42,36 @@ public void start() {
super.start();
}
-
+
@Override
- public void preInstall() {
- super.preInstall();
- executeCommand(VanillaWindowsProcess.PRE_INSTALL_COMMAND, VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND, true);
- if (entity.getConfig(VanillaWindowsProcess.PRE_INSTALL_REBOOT_REQUIRED)) {
- rebootAndWait();
+ public void runPreInstallCommand() {
+ if(Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.PRE_INSTALL_COMMAND)) || Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND))) {
+ executeCommand(VanillaWindowsProcess.PRE_INSTALL_COMMAND, VanillaWindowsProcess.PRE_INSTALL_POWERSHELL_COMMAND, true);
+ if (entity.getConfig(VanillaWindowsProcess.PRE_INSTALL_REBOOT_REQUIRED)) {
+ rebootAndWait();
+ }
}
}
@Override
public void install() {
// TODO: Follow install path of VanillaSoftwareProcessSshDriver
- executeCommand(VanillaWindowsProcess.INSTALL_COMMAND, VanillaWindowsProcess.INSTALL_POWERSHELL_COMMAND, true);
- if (entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
- rebootAndWait();
+ if(Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.INSTALL_COMMAND)) || Strings.isNonBlank(getEntity().getConfig(VanillaWindowsProcess.INSTALL_POWERSHELL_COMMAND))) {
+ executeCommand(VanillaWindowsProcess.INSTALL_COMMAND, VanillaWindowsProcess.INSTALL_POWERSHELL_COMMAND, true);
+ if (entity.getConfig(VanillaWindowsProcess.INSTALL_REBOOT_REQUIRED)) {
--- End diff --
Same here and in the other commands: move the reboot check outside of the outer if block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38081899
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -39,8 +44,11 @@
import com.google.api.client.util.Strings;
import com.google.common.collect.ImmutableList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractSoftwareProcessDriver.class);
--- End diff --
Please change the logger to `LoggerFactory.getLogger(AbstractSoftwareProcessWinRmDriver.class)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37967596
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/test/location/WinRmMachineLocationLiveTest.java ---
@@ -65,7 +65,7 @@ public void setUpClass() throws Exception {
JcloudsLocation loc = (JcloudsLocation) mgmt.getLocationRegistry().resolve("jclouds:aws-ec2:us-west-2", ImmutableMap.of(
"inboundPorts", ImmutableList.of(5985, 3389),
"displayName", "AWS Oregon (Windows)",
- "imageId", "us-west-2/ami-8fd3f9bf",
+ "imageNameRegex", ".*Windows.*2012.*",
--- End diff --
A customer said that they also needed to specify an image owner, in order to avoid a marketplace windows AMI being picked. Can you search for AMIs that match that regex to see if that is a risk?
For example:
brooklyn.location.named.mywindows1.imageOwner=801119661308
brooklyn.location.named.mywindows1.imageNameRegex=Windows_Server-2012-R2_RTM-English-64Bit-Base-.*
This is used within our jclouds integration code (in ComputeServiceRegistryImpl.findComputeService) where we have:
properties.setProperty(PROPERTY_EC2_AMI_QUERY,
"owner-id="+conf.getStringKey("imageOwner")+";state=available;image-type=machine");
(It's a bit ugly having the code there; would be nicer if that were a first-class property on jclouds' AWSEC2TemplateOptions. That's an improvement we could make in jclouds. It would also be nice to have a config key for `imageOwner` rather than just this hard-coded constant being looked up in the config!).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-135374318
Build failure by asfbot was for `RebindJmxFeedTest.testJmxFeedIsPersisted` with "Port already in use: 40131", so is unrelated to this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37967706
--- Diff: software/base/src/test/resources/brooklyn/entity/basic/fast_stdout_blueprint_test.yaml ---
@@ -0,0 +1,40 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+name: Windows stdout
--- End diff --
Not sure about having this yaml file here, given it is never used.
If you do leave it in, then it definitely needs a good comment explaining what it does and what it is useful for.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966477
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/VanillaWindowsProcessWinRmDriver.java ---
@@ -18,12 +18,17 @@
*/
package org.apache.brooklyn.entity.software.base;
+import io.cloudsoft.winrm4j.winrm.WinRmToolResponse;
import org.apache.brooklyn.api.entity.EntityLocal;
import org.apache.brooklyn.core.entity.Attributes;
import org.apache.brooklyn.location.winrm.WinRmMachineLocation;
import org.apache.brooklyn.util.net.UserAndHostAndPort;
+import org.apache.brooklyn.util.text.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class VanillaWindowsProcessWinRmDriver extends AbstractSoftwareProcessWinRmDriver implements VanillaWindowsProcessDriver {
+ Logger LOG = LoggerFactory.getLogger(VanillaWindowsProcessWinRmDriver.class);
--- End diff --
Always declare these `private static final Logger LOG =`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37965260
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -39,8 +44,11 @@
import com.google.api.client.util.Strings;
import com.google.common.collect.ImmutableList;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public abstract class AbstractSoftwareProcessWinRmDriver extends AbstractSoftwareProcessDriver {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractSoftwareProcessDriver.class);
--- End diff --
logger should say `AbstractSoftwareProcessWinRmDriver.class`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/866
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37965215
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessDriver.java ---
@@ -189,7 +187,7 @@ public void start() {
*/
public void preInstall() {}
- public abstract void runPreInstallCommand(String command);
+ public abstract void runPreInstallCommand();
--- End diff --
Same here: the `runPreInstallCommand` should have the same signature as `runPostInstallCommand` etc.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#issuecomment-135365840
@aledsage Thank you for the elaborative comments!
I fixed the addressed issues and added new config for powershell commands for: post-install, pre-launch, post-launch
You can review it again now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38088447
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -119,20 +136,53 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
String regularCommand = getEntity().getConfig(regularCommandKey);
String powershellCommand = getEntity().getConfig(powershellCommandKey);
- if (Strings.isNullOrEmpty(regularCommand) && Strings.isNullOrEmpty(powershellCommand)) {
+ if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
if (allowNoOp) {
return new WinRmToolResponse("", "", 0);
} else {
throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- } else if (!Strings.isNullOrEmpty(regularCommand) && !Strings.isNullOrEmpty(powershellCommand)) {
+ } else if (!Strings.isBlank(regularCommand) && !Strings.isBlank(powershellCommand)) {
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
- if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ ByteArrayOutputStream stdIn = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
+
+ boolean tagWithStd = Tasks.current() != null;
+ if (tagWithStd) {
+ writeToStream(stdIn, Strings.isBlank(regularCommand) ? powershellCommand : regularCommand);
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdIn));
+
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdOut));
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDERR, stdErr));
+ }
+ }
+
+ WinRmToolResponse response;
+ if (Strings.isBlank(regularCommand)) {
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (tagWithStd) {
+ if (BrooklynTaskTags.stream(Tasks.current(), BrooklynTaskTags.STREAM_STDOUT)==null) {
--- End diff --
I've followed the pattern from here https://github.com/apache/incubator-brooklyn/blob/master/software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessSshDriver.java#L262
There it is used check for STDOUT availability
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r38082876
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -129,10 +132,40 @@ protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey,
throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommandKey.getName(), powershellCommandKey.getName()));
}
+ WinRmToolResponse response;
if (Strings.isNullOrEmpty(regularCommand)) {
- return getLocation().executePsScript(ImmutableList.of(powershellCommand));
+ response = getLocation().executePsScript(ImmutableList.of(powershellCommand));
} else {
- return getLocation().executeScript(ImmutableList.of(regularCommand));
+ response = getLocation().executeScript(ImmutableList.of(regularCommand));
+ }
+
+ if (Tasks.current()!=null) {
--- End diff --
I'd cache whether we've made use of these stdOut/stdErr, rather than calling `if (Tasks.current()!=null)` again. If we added those tags previously then call `writeToStream` (rather than also looking up the `BrooklynTaskTags.stream()` again - no need to do that).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Winrm commands stdin, stdout, std...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/866#discussion_r37966943
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/VanillaSoftwareProcessStreamsIntegrationTest.java ---
@@ -59,8 +39,24 @@ public void setUp() throws Exception {
}
@Test(groups = "Integration")
+ @Override
public void testGetsStreams() {
- Map<String, String> cmds = ImmutableMap.<String, String>builder()
+ VanillaSoftwareProcess entity = app.createAndManageChild(EntitySpec.create(VanillaSoftwareProcess.class)
+ .configure(VanillaSoftwareProcess.PRE_INSTALL_COMMAND, "echo " + getCommands().get("pre-install-command"))
+ .configure(VanillaSoftwareProcess.INSTALL_COMMAND, "echo " + getCommands().get("ssh: installing.*"))
+ .configure(VanillaSoftwareProcess.POST_INSTALL_COMMAND, "echo " + getCommands().get("post-install-command"))
+ .configure(VanillaSoftwareProcess.CUSTOMIZE_COMMAND, "echo " + getCommands().get("ssh: customizing.*"))
+ .configure(VanillaSoftwareProcess.PRE_LAUNCH_COMMAND, "echo " + getCommands().get("pre-launch-command"))
+ .configure(VanillaSoftwareProcess.LAUNCH_COMMAND, "echo " + getCommands().get("ssh: launching.*"))
+ .configure(VanillaSoftwareProcess.POST_LAUNCH_COMMAND, "echo " + getCommands().get("post-launch-command"))
+ .configure(VanillaSoftwareProcess.CHECK_RUNNING_COMMAND, "echo true"));
+ app.start(ImmutableList.of(localhost));
+ assertStreams(entity);
--- End diff --
I'd pass the `cmds` as an argument of `assertStreams(...)`. That slightly decreases the coupling between the various methods. (very minor point).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---