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/09/18 02:49:10 UTC
[GitHub] incubator-brooklyn pull request: Capture exit code in AbstractSoft...
GitHub user bostko opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/906
Capture exit code in AbstractSoftwareProcessWinrmDriver
- Refactor AbstractSoftwareProcessWinrmDriver implementation like AbstractSoftwareProcessSshDriver
- ScriptHelperUnitTest exit code test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/incubator-brooklyn winrm_refactoring_execution
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/906.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 #906
----
commit ec5e821733d9b2cc201a1553abd26945aded0f3a
Author: Valentin Aitken <va...@cloudsoftcorp.com>
Date: 2015-09-09T19:47:24Z
Capture exit code in AbstractSoftwareProcessWinrmDriver
- Refactor AbstractSoftwareProcessWinrmDriver implementation like AbstractSoftwareProcessSshDriver
- ScriptHelper exit code test
----
---
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: Capture exit code in AbstractSoft...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/906
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838978
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/WinRmExecuteHelperUnitTest.java ---
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingWinRmSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class WinRmExecuteHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingWinRmSoftwareProcessDriver nativeWindowsScriptRunner = mock(DoNothingWinRmSoftwareProcessDriver.class);
+
+ WinRmExecuteHelper scriptHelper = new WinRmExecuteHelper(nativeWindowsScriptRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "WinRmExecuteHelper doesn't return zero code");
--- End diff --
As commented above, don't throw away what the actual value was; I personally wouldn't worry about the custom error message as it will be obvious from the line of code and from the test name.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39920429
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -134,17 +189,20 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
}
- protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
- String regularCommand = getEntity().getConfig(regularCommandKey);
- String powershellCommand = getEntity().getConfig(powershellCommandKey);
- if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
+ public int executeCommandInTask(String regularCommand, String powershellCommand, String phase, Boolean allowNoOp) {
--- End diff --
It is surprising that the presence of the `Boolean allowNoOp` in the overloaded method causes it to go down a completely different code path, not calling `newScript(...)...` in the same was as the other `executeCommandInTask` method.
Do you really mean for this `executeCommandInTask` method that it is *not* executed in a task? Or should it do the no-op guard, and then delegate to the other `executeCommandInTask` method?
---
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: Capture exit code in AbstractSoft...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/906#issuecomment-142006511
This [1] requires java 8. Additionally, `ScriptHelperUnitTest.testTaskNoGatherOutput` fails!
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838605
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/lifecycle/WinRmExecuteHelper.java ---
@@ -0,0 +1,217 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import org.apache.brooklyn.api.entity.Entity;
+import org.apache.brooklyn.api.mgmt.ExecutionContext;
+import org.apache.brooklyn.api.mgmt.Task;
+import org.apache.brooklyn.api.mgmt.TaskQueueingContext;
+import org.apache.brooklyn.core.mgmt.BrooklynTaskTags;
+import org.apache.brooklyn.util.core.task.DynamicTasks;
+import org.apache.brooklyn.util.core.task.TaskBuilder;
+import org.apache.brooklyn.util.core.task.Tasks;
+import org.apache.brooklyn.util.stream.Streams;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.annotation.Nullable;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.concurrent.Callable;
+
+import static java.lang.String.format;
+
+/**
+ * <code>org.apache.brooklyn.entity.software.base.lifecycle.ScriptHelper</code> analog for WinRM
+ */
+public class WinRmExecuteHelper {
+ public static final Logger LOG = LoggerFactory.getLogger(WinRmExecuteHelper.class);
+
+ private Entity entity;
+ private Task<Integer> task;
+
+ protected final NativeWindowsScriptRunner runner;
+ public final String summary;
+
+ private String command;
+ private String psCommand;
+
+ @SuppressWarnings("rawtypes")
+ protected final Map flags = new LinkedHashMap();
+ protected Predicate<? super Integer> resultCodeCheck = Predicates.alwaysTrue();
+ protected ByteArrayOutputStream stdout, stderr, stdin;
+
+ public WinRmExecuteHelper(NativeWindowsScriptRunner runner, String summary) {
+ this.runner = runner;
+ this.summary = summary;
+ }
+
+ public WinRmExecuteHelper setCommand(String command) {
+ this.command = command;
+ return this;
+ }
+
+ public WinRmExecuteHelper setPsCommand(String psCommand) {
+ this.psCommand = psCommand;
+ return this;
+ }
+
+ protected Entity getEntity() {
+ return entity;
+ }
+
+ /** queues the task for execution if we are in a {@link TaskQueueingContext} (e.g. EffectorTaskFactory);
+ * or if we aren't in a queueing context, it will submit the task (assuming there is an {@link ExecutionContext}
+ * _and_ block until completion, throwing on error */
+ @Beta
+ public Task<Integer> queue() {
+ return DynamicTasks.queueIfPossible(newTask()).orSubmitAndBlock().getTask();
+ }
+
+ /** creates a task which will execute this script; note this can only be run once per instance of this class */
+ public synchronized Task<Integer> newTask() {
+ if (task!=null) throw new IllegalStateException("task can only be generated once");
+ TaskBuilder<Integer> tb = Tasks.<Integer>builder().displayName("winrm: "+summary).body(
+ new Callable<Integer>() {
+ public Integer call() throws Exception {
+ return executeInternal();
+ }
+ });
+
+ try {
+ ByteArrayOutputStream stdin = new ByteArrayOutputStream();
+ stdin.write((command != null ? command : psCommand).getBytes());
+ tb.tag(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdin));
+ } catch (IOException e) {
+ LOG.warn("Error registering stream "+BrooklynTaskTags.STREAM_STDIN+" on "+tb+": "+e, e);
+ }
+
+ Map flags = getFlags();
+
+ Map<?,?> env = (Map<?,?>) flags.get("env");
+ if (env!=null) {
+ // if not explicitly set, env will come from getShellEnv in AbstractSoftwareProcessSshDriver.execute,
+ // which will also update this tag appropriately
+ tb.tag(BrooklynTaskTags.tagForEnvStream(BrooklynTaskTags.STREAM_ENV, env));
+ }
+
+ if (gatherOutput) {
+ stdout = new ByteArrayOutputStream();
+ tb.tag(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdout));
--- End diff --
Why do we now have this code here and in `AbstractSoftwareProcessWinRmDriver.executeCommand`? Is there a way to just have it in one place? e.g. could `AbstractSoftwareProcessWinRmDriver.executeCommand` use the `WinRmExecuteHelper` somehow?
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838816
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/ScriptHelperUnitTest.java ---
@@ -0,0 +1,63 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ScriptHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "ScriptHelper doesn't return zero code");
--- End diff --
Better to use `Asserts.assetEquals(scriptHelper.executeInternal(), 0)`. With your code, the error won't tell us what the actual exit code is, which hides information.
---
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: Capture exit code in AbstractSoft...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/906#issuecomment-141405379
We need live tests that the different exit codes are actually respected, when thrown by WinRM powershell or native commands - those are vital for having confidence that the WinRM stuff is working.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39839107
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/WinRmExecuteHelperUnitTest.java ---
@@ -0,0 +1,62 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingWinRmSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class WinRmExecuteHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingWinRmSoftwareProcessDriver nativeWindowsScriptRunner = mock(DoNothingWinRmSoftwareProcessDriver.class);
+
+ WinRmExecuteHelper scriptHelper = new WinRmExecuteHelper(nativeWindowsScriptRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "WinRmExecuteHelper doesn't return zero code");
+ }
+
+ @Test
+ public void testNonZeroExitCode() {
+ DoNothingWinRmSoftwareProcessDriver nativeWindowsScriptRunner = mock(DoNothingWinRmSoftwareProcessDriver.class);
+ when(nativeWindowsScriptRunner.executeCommand(any(Map.class), any(String.class), any(String.class), any(String.class), any(Boolean.class))).thenReturn(1);
+
+ WinRmExecuteHelper scriptHelper = new WinRmExecuteHelper(nativeWindowsScriptRunner, "test-zero-code-task")
+ .setCommand(NON_ZERO_CODE_COMMAND);
+ Asserts.assertTrue(scriptHelper.executeInternal() != 0, "WinRmExecuteHelper return zero code for non-zero code task");
+ }
+
+ @Test(expectedExceptions = IllegalStateException.class)
+ public void testNonZeroExitCodeException() {
--- End diff --
This method's body looks the same as `testNonZeroExitCode`. Given you are actually asserting with `expectedExceptions` above, then strip out the extra code (e.g. the `assertTrue` at the end of this method).
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39869652
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/ScriptHelperUnitTest.java ---
@@ -0,0 +1,63 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ScriptHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "ScriptHelper doesn't return zero code");
--- End diff --
We should think about extending `org.apache.brooklyn.test.Asserts` from `org.testng.Assert`
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39970328
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessStreamsTest.java ---
@@ -105,8 +105,8 @@ protected String getStreamOrFail(Task<?> task, String streamType) {
String stdin = getStreamOrFail(subTask, BrooklynTaskTags.STREAM_STDIN);
String stdout = getStreamOrFail(subTask, BrooklynTaskTags.STREAM_STDOUT);
String stderr = getStreamOrFail(subTask, BrooklynTaskTags.STREAM_STDERR);
- String env = getStreamOrFail(subTask, BrooklynTaskTags.STREAM_ENV);
- String msg = "stdin="+stdin+"; stdout="+stdout+"; stderr="+stderr+"; env="+env;
+// String env = getStreamOrFail(subTask, BrooklynTaskTags.STREAM_ENV);
--- End diff --
I strongly dislike commented out that has no comment to say why it's commented out and why it might be useful in the future (versus it being deleted).
---
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: Capture exit code in AbstractSoft...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/906#issuecomment-141980383
Thanks @bostko - nerging now. We can iterate on it in additional PRs as required.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838423
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -134,18 +189,23 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
}
- protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
- String regularCommand = getEntity().getConfig(regularCommandKey);
- String powershellCommand = getEntity().getConfig(powershellCommandKey);
+ public int executeCommand(String regularCommand, String powershellCommand, String phase, Boolean allowNoOp) {
+ return executeCommand(MutableMap.of(), regularCommand, powershellCommand, phase, allowNoOp);
+ }
+
+ @Override
+ public Integer executeCommand(Map flags2, String regularCommand, String powershellCommand, String phase, Boolean allowNoOp) {
if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
if (allowNoOp) {
- return new WinRmToolResponse("", "", 0);
+ return new WinRmToolResponse("", "", 0).getStatusCode();
} else {
- throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
+ throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommand, 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()));
+ throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommand, powershellCommand));
}
+ Map flags = Maps.newLinkedHashMap();
--- End diff --
I don't understand how you are using `flags` and `flags2`. You put things into `flags2`, but it looks like the map is never used?
Now that you don't return `WinRmToolResponse`, we need another way for someone to get the stdout/stderr. The way ssh does it is to allow someone to pass in an `OutputStream` in the `flags2` parameter, using the keys `out` and `err`. If they are passed in, then they are populated with the stdout/stderr. That approach is more fiddly code than is really required for this use-case (where the stdout/stderr is not available incrementally anyway) but we should make sure that the stdout/stderr is accessible.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39833499
--- Diff: core/src/main/java/org/apache/brooklyn/location/winrm/WinRmMachineLocation.java ---
@@ -18,32 +18,6 @@
*/
package org.apache.brooklyn.location.winrm;
-import static com.google.common.base.Preconditions.checkNotNull;
--- End diff --
I prefer not having changes that just re-order imports.
Also, the vast majority of our code uses the Eclipse convention for ordering imports (this looks like the Intellij default). I believe that (some) Intellij users change their settings to use the Eclipse ordering, for Brooklyn.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838904
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/ScriptHelperUnitTest.java ---
@@ -0,0 +1,63 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ScriptHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "ScriptHelper doesn't return zero code");
+ }
+
+ @Test
+ public void testNonZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+ when(sshRunner.execute(any(Map.class), any(List.class), any(String.class))).thenReturn(1);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task")
+ .body.append(NON_ZERO_CODE_COMMAND);
+ Asserts.assertTrue(scriptHelper.executeInternal() != 0, "ScriptHelper return zero code for non-zero code task");
+ }
+
+ @Test(expectedExceptions = IllegalStateException.class)
+ public void testNonZeroExitCodeException() {
--- End diff --
This method looks the same as `testNonZeroExitCode`. Should it be asserting about an exception being thrown by the mock, rather than the mock returning 1?
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39839095
--- Diff: software/base/src/test/java/org/apache/brooklyn/entity/software/base/lifecycle/ScriptHelperUnitTest.java ---
@@ -0,0 +1,63 @@
+/*
+ * 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.
+ */
+package org.apache.brooklyn.entity.software.base.lifecycle;
+
+import org.apache.brooklyn.entity.software.base.DoNothingSoftwareProcessDriver;
+import org.apache.brooklyn.test.Asserts;
+import org.testng.annotations.Test;
+
+import java.util.List;
+import java.util.Map;
+
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class ScriptHelperUnitTest {
+ private static final String NON_ZERO_CODE_COMMAND = "false";
+
+ @Test
+ public void testZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task");
+ Asserts.assertTrue(scriptHelper.executeInternal() == 0, "ScriptHelper doesn't return zero code");
+ }
+
+ @Test
+ public void testNonZeroExitCode() {
+ DoNothingSoftwareProcessDriver sshRunner = mock(DoNothingSoftwareProcessDriver.class);
+ when(sshRunner.execute(any(Map.class), any(List.class), any(String.class))).thenReturn(1);
+
+ ScriptHelper scriptHelper = new ScriptHelper(sshRunner, "test-zero-code-task")
+ .body.append(NON_ZERO_CODE_COMMAND);
+ Asserts.assertTrue(scriptHelper.executeInternal() != 0, "ScriptHelper return zero code for non-zero code task");
+ }
+
+ @Test(expectedExceptions = IllegalStateException.class)
+ public void testNonZeroExitCodeException() {
--- End diff --
This method's body looks the same as `testNonZeroExitCode`. Given you are actually asserting with `expectedExceptions` above, then strip out the extra code (e.g. the `assertTrue` at the end of this method).
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39833656
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -60,10 +66,27 @@ public AbstractSoftwareProcessWinRmDriver(EntityLocal entity, WinRmMachineLocati
entity.setAttribute(WINDOWS_PASSWORD, location.config().get(WinRmMachineLocation.PASSWORD));
}
+ /** @see #newScript(Map, String) */
+ protected WinRmExecuteHelper newScript(String phase) {
+ return newScript(Maps.<String, Object>newLinkedHashMap(), phase);
+ }
+
+ protected WinRmExecuteHelper newScript(Map<String, ?> flags, String phase) {
+ if (!Entities.isManaged(getEntity()))
+ throw new IllegalStateException(getEntity() + " is no longer managed; cannot create script to run here (" + phase + ")");
+
+ WinRmExecuteHelper s = new WinRmExecuteHelper(this, phase + " " + elvis(entity, this));
+ return s;
+ }
+
@Override
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);
+ executeCommand(
--- End diff --
Why is this method using `executeCommand`, but the other methods use `executeNativeOrPsCommand`?
---
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: Capture exit code in AbstractSoft...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/906#issuecomment-141406660
I'm concerned that we've made the code more complicated by adding the `WinRmExecuteHelper.java`, but without it having the same functionality as the ssh version (e.g. it's impossible to get the stdout/stderr back when you use it).
We don't actually need the `WinRmExecuteHelper` currently (because things are called from yaml) so we can defer that, but if we're deferring it then it feels like we could have deferred adding this class just now.
With this change, we are much of the way there to having the more powerful execution helper, but we have lost functionality compared to where we were before (i.e. you can't get the stdout/stderr if writing Java).
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39967665
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -134,37 +187,40 @@ protected void createDirectory(String directoryName, String summaryForLogging) {
getLocation().executePsScript("New-Item -path \"" + directoryName + "\" -type directory -ErrorAction SilentlyContinue");
}
- protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey, ConfigKey<String> powershellCommandKey, boolean allowNoOp) {
- String regularCommand = getEntity().getConfig(regularCommandKey);
- String powershellCommand = getEntity().getConfig(powershellCommandKey);
- if (Strings.isBlank(regularCommand) && Strings.isBlank(powershellCommand)) {
+ @Override
+ public Integer executeNativeOrPsCommand(Map flags, String regularCommand, String powerShellCommand, String phase, Boolean allowNoOp) {
+ if (Strings.isBlank(regularCommand) && Strings.isBlank(powerShellCommand)) {
if (allowNoOp) {
- return new WinRmToolResponse("", "", 0);
+ return new WinRmToolResponse("", "", 0).getStatusCode();
} else {
- throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommandKey.getName(), powershellCommandKey.getName()));
+ throw new IllegalStateException(String.format("Exactly one of %s or %s must be set", regularCommand, 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()));
+ } else if (!Strings.isBlank(regularCommand) && !Strings.isBlank(powerShellCommand)) {
+ throw new IllegalStateException(String.format("%s and %s cannot both be set", regularCommand, powerShellCommand));
}
ByteArrayOutputStream stdIn = new ByteArrayOutputStream();
- ByteArrayOutputStream stdOut = new ByteArrayOutputStream();
- ByteArrayOutputStream stdErr = new ByteArrayOutputStream();
+ ByteArrayOutputStream stdOut = flags.get("out") != null ? (ByteArrayOutputStream)flags.get("out") : new ByteArrayOutputStream();
--- End diff --
We should not assume that the caller has passed in a `ByteArrayOutputStream` when they have populated the `out` or `err` key in `flags`.
Let's not worry about it in this PR though. I see that `AbstractSoftwareProcessSshDriver` is bad as well, but in a different way. It overwrites any stdout and stderr keys (if any) that were passed in by the user.
---
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: Capture exit code in AbstractSoft...
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/906#discussion_r39838215
--- Diff: software/base/src/main/java/org/apache/brooklyn/entity/software/base/AbstractSoftwareProcessWinRmDriver.java ---
@@ -154,11 +214,11 @@ protected WinRmToolResponse executeCommand(ConfigKey<String> regularCommandKey,
Task<?> currentTask = Tasks.current();
if (currentTask != null) {
writeToStream(stdIn, Strings.isBlank(regularCommand) ? powershellCommand : regularCommand);
- Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdIn));
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDIN, stdIn)); flags.put("stdin", stdIn);
if (BrooklynTaskTags.stream(currentTask, BrooklynTaskTags.STREAM_STDOUT)==null) {
- Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdOut));
- Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDERR, stdErr));
+ Tasks.addTagDynamically(BrooklynTaskTags.tagForStreamSoft(BrooklynTaskTags.STREAM_STDOUT, stdOut)); flags.put("stdout", stdOut);
--- End diff --
Can you separate these commands onto multiple lines - it risks being missed by someone reading the code when at the end of a long line like this. Same for the code above.
---
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.
---