You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by CMoH <gi...@git.apache.org> on 2015/07/17 18:10:31 UTC
[GitHub] incubator-brooklyn pull request: Fix SshMachineLocation unable to ...
GitHub user CMoH opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/757
Fix SshMachineLocation unable to connect
Details in commit message.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/CMoH/incubator-brooklyn bug_ssh_port_null
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/757.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 #757
----
commit 29da8c723fc90d192e67c26c538abe34a2d7c51a
Author: Ciprian Ciubotariu <ch...@gmx.net>
Date: 2015-07-17T15:41:32Z
Fix SshMachineLocation unable to connect without port setting
ShellAbstractTool.getOptionalVal is unable to coerce null to integer
type, throwing NullPointerException. This manifests as
SshMachineLocation being unable to connect to any host that has no
explicit port setting.
----
---
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: Fix SshMachineLocation unable to ...
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/757#discussion_r35413249
--- Diff: core/src/main/java/brooklyn/util/internal/ssh/ShellAbstractTool.java ---
@@ -105,7 +105,7 @@ protected static Boolean hasVal(Map<String,?> map, ConfigKey<?> keyC) {
public static <T> T getOptionalVal(Map<String,?> map, ConfigKey<T> keyC) {
if (keyC==null) return null;
String key = keyC.getName();
- if (map!=null && map.containsKey(key)) {
+ if (map!=null && map.containsKey(key) && map.get(key) != null) {
--- End diff --
`ConfigBag` returns the default if the value is null, so I think it's nice to have the same behaviour 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.
---
[GitHub] incubator-brooklyn pull request: Fix SshMachineLocation unable to ...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#issuecomment-124083589
You are right. I update the PR.
The second commit avoids adding the null port value to the config map. I've tested it and both fixes perform as intended.
---
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: Fix SshMachineLocation unable to ...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#issuecomment-124025460
The null value comes from ByonLocationResolver.extractConfig(), at https://github.com/apache/incubator-brooklyn/blob/master/core/src/main/java/brooklyn/location/basic/ByonLocationResolver.java#L97
Integer port = (Integer) TypeCoercions.coerce(config.getStringKey("port"), Integer.class);
When the port is missing from the config bag, null gets coerced to null and transitions through until the call to getOptionalVal().
In my understanding, an "optional" value should provide the default when missing - which is the exact meaning of null. Whether it's missing from the property map, or it is mapped to null should make no difference.
The opposite interpretation would be to not put nulls in maps, which means the ByonLocationResolver (and all similar client code) must implement such null checks.
---
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: Fix SshMachineLocation unable to ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#issuecomment-124084596
Good fixes, can be merged.
---
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: Fix SshMachineLocation unable to ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#issuecomment-124004612
What are the conditions that lead to the problem? Why does the location put a null value in the config?
Could there be some bigger issue hiding?
---
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: Fix SshMachineLocation unable to ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/757
---
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: Fix SshMachineLocation unable to ...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#issuecomment-124032299
The changes leading to this were merged recently - https://github.com/apache/incubator-brooklyn/pull/721. It's surprising to have null values in the map, so might be worth fixing the new code as well. Regardless your change is valid and agree that null values should cause the default being used. What I'd like to see is the same guards to the other `getOptionalVal` overloading implementation.
---
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: Fix SshMachineLocation unable to ...
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/757#discussion_r35317252
--- Diff: core/src/main/java/brooklyn/location/basic/ByonLocationResolver.java ---
@@ -96,7 +96,11 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, brooklyn.
String user = (String) config.getStringKey("user");
Integer port = (Integer) TypeCoercions.coerce(config.getStringKey("port"), Integer.class);
Class<? extends MachineLocation> locationClass = OS_TO_MACHINE_LOCATION_TYPE.get(config.get(OS_FAMILY));
-
+
+ MutableMap<String, Object> defaultProps = MutableMap.of();
+ if (user != null) defaultProps.add("user", user);
--- End diff --
`MutableMap` supports `addIfNotNull` to avoid the check.
---
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: Fix SshMachineLocation unable to ...
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/757#discussion_r35395212
--- Diff: core/src/main/java/brooklyn/util/internal/ssh/ShellAbstractTool.java ---
@@ -105,7 +105,7 @@ protected static Boolean hasVal(Map<String,?> map, ConfigKey<?> keyC) {
public static <T> T getOptionalVal(Map<String,?> map, ConfigKey<T> keyC) {
if (keyC==null) return null;
String key = keyC.getName();
- if (map!=null && map.containsKey(key)) {
+ if (map!=null && map.containsKey(key) && map.get(key) != null) {
--- End diff --
I'll go with this and merge it.
However, I'm slightly hesitant: in some places, it's nice to be able to explicitly supply null so that it clears the value rather than it using the default. For example, in the use of JcloudsLocation I want to be able to set the privateKeyFile to none, rather than it picking the default of "~/.ssh/id_rsa". But I could probably still do that with a blank string. So not sure if there are any valid use-cases for explicitly setting 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: Fix SshMachineLocation unable to ...
Posted by CMoH <gi...@git.apache.org>.
Github user CMoH commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/757#discussion_r35317360
--- Diff: core/src/main/java/brooklyn/location/basic/ByonLocationResolver.java ---
@@ -96,7 +96,11 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, brooklyn.
String user = (String) config.getStringKey("user");
Integer port = (Integer) TypeCoercions.coerce(config.getStringKey("port"), Integer.class);
Class<? extends MachineLocation> locationClass = OS_TO_MACHINE_LOCATION_TYPE.get(config.get(OS_FAMILY));
-
+
+ MutableMap<String, Object> defaultProps = MutableMap.of();
+ if (user != null) defaultProps.add("user", user);
--- End diff --
Thanks - I'll update again! :)
---
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.
---