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/10 01:37:45 UTC
[GitHub] incubator-brooklyn pull request: SshTool choice
GitHub user aledsage opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1013
SshTool choice
Adds SshMachineLocation.SSH_TOOL_CLASS
- Deprecates SshTool.PROP_TOOL_CLASS
- Adds tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/incubator-brooklyn refactor/ssh-tool-choice
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1013.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 #1013
----
commit 2bdf4ab9555685eab73efc9aea67f82a04954b4f
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-09T08:45:06Z
Make SshMachineLocation.LOG private
commit b43e214ab07313414b1ff3ca7d23c2317eed2160
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-09T08:45:53Z
Deprecate public LocalhostMachineProvisioningLocation.LOG
So can make it private
commit 9c14b63b5da508586055cd9386e304fe399045df
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-10T00:34:20Z
Adds ConfigKeys.newConfigKeyWithoutPrefix
commit 1d2024e1fefc66c749fb9fed28a3931d84b9656a
Author: Aled Sage <al...@gmail.com>
Date: 2015-11-10T00:36:05Z
Adds SshMachineLocation.SSH_TOOL_CLASS
- Deprecates SshTool.PROP_TOOL_CLASS
- Adds tests
----
---
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: SshTool choice
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#discussion_r44434315
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/SshTool.java ---
@@ -49,7 +50,14 @@
* These keys are detected from entity/global config and automatically applied to ssh executions. */
public static final String BROOKLYN_CONFIG_KEY_PREFIX = "brooklyn.ssh.config.";
- public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey("tool.class", "SshTool implementation to use", null);
+ /**
+ * @deprecated since 0.9.0; use {@link SshMachineLocation#SSH_TOOL_CLASS}
+ */
+ @Deprecated
+ public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey(
--- End diff --
agree wtih @bostko about pattern where we share the static config key definition; but this particular key name is horrible so +1 to the deprecation.
---
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: SshTool choice
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#issuecomment-156099447
Thanks all. I've incorporated this feedback (and added comments to the code about different keys for WinRmTool versus SshTool implementations, and about why the config key is not to be defined in SshTool).
I've also rebased against master.
Waiting for jenkins to confirm that it builds, and then will merge.
---
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: SshTool choice
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/1013#discussion_r44552398
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/SshTool.java ---
@@ -49,7 +50,14 @@
* These keys are detected from entity/global config and automatically applied to ssh executions. */
public static final String BROOKLYN_CONFIG_KEY_PREFIX = "brooklyn.ssh.config.";
- public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey("tool.class", "SshTool implementation to use", null);
+ /**
+ * @deprecated since 0.9.0; use {@link SshMachineLocation#SSH_TOOL_CLASS}
+ */
+ @Deprecated
+ public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey(
--- End diff --
We need a different key name for the class name of the WinRmTool and the SshTool. We'll use this by writing some configuration on the location (e.g. in brooklyn.properties on a named location), and will want to supply an implementation of WinRmTool and an implementation of SshTool - hence we need a different key name for each.
My reasoning for not having it in SshTool is that the config there is about config the SshTool understands. By the time you have an SshTool, then it has been instantiated. The place that instantiates the tool is SshMachineLocation, so that feels like a good place to define this config key. I also defined it in BrooklynConfigKeys, but with the common prefix "brooklyn.ssh.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: SshTool choice
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#issuecomment-155437725
Looks good to me as well.
---
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: SshTool choice
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/1013#discussion_r44389092
--- Diff: core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/SshTool.java ---
@@ -49,7 +50,14 @@
* These keys are detected from entity/global config and automatically applied to ssh executions. */
public static final String BROOKLYN_CONFIG_KEY_PREFIX = "brooklyn.ssh.config.";
- public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey("tool.class", "SshTool implementation to use", null);
+ /**
+ * @deprecated since 0.9.0; use {@link SshMachineLocation#SSH_TOOL_CLASS}
+ */
+ @Deprecated
+ public static final ConfigKey<String> PROP_TOOL_CLASS = newStringConfigKey(
--- End diff --
Can you explain why did you deprecate this?
It is good if this key is in a generic location like `SshTool`. I was thinking about refactoring Winrm support to follow the same pattern as it is for ssh, if so we could implement this dynamic sshTool loading with the same keys and the same API.
In regard with this please check https://github.com/apache/incubator-brooklyn/pull/950
---
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: SshTool choice
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#discussion_r44434065
--- Diff: core/src/main/java/org/apache/brooklyn/core/config/ConfigKeys.java ---
@@ -157,6 +157,14 @@ public static PortAttributeSensorAndConfigKey newPortSensorAndConfigKey(String n
return newConfigKeyRenamed(prefix+key.getName(), key);
}
+ public static <T> ConfigKey<T> newConfigKeyWithoutPrefix(String prefix, ConfigKey<T> key) {
--- End diff --
`newConfigKeyWithPrefixRemoved` ?
---
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: SshTool choice
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#issuecomment-155490937
agree with others, minor comments, then good to merge
---
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: SshTool choice
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#issuecomment-155416984
LGTM, 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: SshTool choice
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#discussion_r44403766
--- Diff: core/src/main/java/org/apache/brooklyn/location/localhost/LocalhostMachineProvisioningLocation.java ---
@@ -73,6 +73,7 @@
private static final long serialVersionUID = -7791239672433897762L;
+ /** @deprecated since 0.9.0; shouldn't be public */
--- End diff --
Add `@Deprecated` annotation as well?
---
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: SshTool choice
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1013#discussion_r44403804
--- Diff: core/src/main/java/org/apache/brooklyn/location/ssh/SshMachineLocation.java ---
@@ -135,13 +136,17 @@
public class SshMachineLocation extends AbstractLocation implements MachineLocation, PortSupplier, WithMutexes, Closeable {
/** @deprecated since 0.7.0 shouldn't be public */
--- End diff --
Remove `@deprecated` marker now that these are private.
---
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: SshTool choice
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/1013
---
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.
---