You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2015/11/18 23:08:59 UTC
[GitHub] incubator-brooklyn pull request: Adds WinRmTool interface
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1045
Adds WinRmTool interface
We want the interface/class within Brooklyn so that the rest of the code doesn't depend on winrm4j directly.
We also want to be able to swap out the Winrm4jTool for an alternative implementation, just through configuration.
Note that the unqualified interface name / class name are the same as the names in `io.cloudsoft.winrm4j.winrm.WinRmTool` and ` io.cloudsoft.winrm4j.winrm.WinRmToolResponse`. Is that a good idea? Any better suggestions for names?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/WinRmTool-interface
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1045.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 #1045
----
commit 42c1113258bb66ba98c7fcde6c304dad4faed38c
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-18T15:33:07Z
Adds WinRmTool interface
commit f8c7673604ebc572f3eb3bb01d0dfe53a954c8e6
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-18T15:33:22Z
Fix javadoc typo in SshTool
----
---
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: Adds WinRmTool interface
Posted by ygy <gi...@git.apache.org>.
Github user ygy commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1045#issuecomment-159291985
@aledsage
LGTM.
There are some Live tests failings, but they are not related to this PR. Most of them will just need adjusting the asserts
---
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: Adds WinRmTool interface
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1045#issuecomment-159287775
Very good start for putting alternative WinRm implementations!
As part of this refactoring I want to remind you to review #950 .
My opinion is that we have to have the basic `executeCommand(String)` signature instead of `executeScript(String)`.
If we want to execute scripts is supposed to be configurable e.g. error logging, exit on first error. This requires extra work which shouldn't be mandatory.
Interesting that `SshTool` has `executeCommands(List<String>...)` and `executeScript(List<String>,...)`
Basically I think that we have to have only `executeCommand(String)` or `executeCommands(List<String>)` in `WinRmTool` interface.
---
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: Adds WinRmTool interface
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1045
---
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: Adds WinRmTool interface
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1045#issuecomment-159407556
Looks good, merging.
---
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: Adds WinRmTool interface
Posted by hzbarcea <gi...@git.apache.org>.
Github user hzbarcea commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1045#issuecomment-158690738
I didn't review this thoroughly and this is a comment related less to this particular PR as to a more general design aspect. I think the bigger issue is the fact that we handle ```windows``` as a special case (see all the ```if (windows)``` constructs in ```./locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java```) is what I would address first. It may inform the way we implement the WinRm support.
I have a more concrete proposal, but still not 100% fleshed out. I will post it on the dev@ list. If you want this PR merged, I am ok with it, but I think we'll revisit it in not far in the future.
---
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: Adds WinRmTool interface
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1045#issuecomment-159071176
Thanks @hzbarcea
The `JcloudsLocation` code needs a complete re-write. It's too big, too many if statements, too much cloud-specific code in the middle of methods (e.g. if aws, or if docker), not unit testable, a fairly ad hoc set of injection points for customising the provisioning behaviour, etc. I also agree about the way `if windows` being a big code smell.
I think that at some level of our code, we'll hit the use of an ssh-client or a winrm-client (unless we went all-in with chef/puppet/salt etc). Therefore I think the bulk of this PR is not related to the problems you refer to. We want to remove the dependency on `io.cloudsoft.winrm4j` that had leaked into too many parts of the code.
As for `WinRmMachineLocation`, I think this code improves it by putting the right (or at least more right) responsibilities into the winrm-client.
I'd therefore like us to merge it before we tackle the big `JcloudsLocation` and other such classes.
---
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: Adds WinRmTool interface
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/1045#discussion_r45739428
--- Diff: software/winrm/src/main/java/org/apache/brooklyn/util/core/internal/winrm/WinRmTool.java ---
@@ -0,0 +1,74 @@
+/*
+ * 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.util.core.internal.winrm;
+
+import static org.apache.brooklyn.core.config.ConfigKeys.newConfigKey;
+import static org.apache.brooklyn.core.config.ConfigKeys.newIntegerConfigKey;
+import static org.apache.brooklyn.core.config.ConfigKeys.newStringConfigKey;
+
+import java.io.InputStream;
+import java.util.List;
+
+import org.apache.brooklyn.config.ConfigKey;
+import org.apache.brooklyn.core.config.ConfigKeys;
+import org.apache.brooklyn.core.entity.BrooklynConfigKeys;
+import org.apache.brooklyn.util.time.Duration;
+
+import com.google.common.annotations.Beta;
+import com.google.common.base.Preconditions;
+
+@Beta
+public interface WinRmTool {
+
+ /** Public-facing global config keys for Brooklyn are defined in ConfigKeys,
+ * and have this prefix prepended to the config keys in this class.
+ * These keys are detected from entity/global config and automatically applied to ssh executions. */
+ String BROOKLYN_CONFIG_KEY_PREFIX = Preconditions.checkNotNull(BrooklynConfigKeys.BROOKLYN_WINRM_CONFIG_KEY_PREFIX,
+ "static final initializer classload ordering problem");
+
+ ConfigKey<String> PROP_HOST = newStringConfigKey("host", "Host to connect to (required)", null);
--- End diff --
Do you have to put `null` here?
---
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.
---