You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by iyovcheva <gi...@git.apache.org> on 2017/05/21 22:44:42 UTC
[GitHub] brooklyn-server pull request #690: External ip config for winrm byon locatio...
GitHub user iyovcheva opened a pull request:
https://github.com/apache/brooklyn-server/pull/690
External ip config for winrm byon location
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iyovcheva/brooklyn-server feature/external-ip
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/690.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 #690
----
commit f0c9dd676cc4afac76b1c79f00d84f8183800b62
Author: Ivana Yovcheva <iv...@gmail.com>
Date: 2017-05-18T00:11:55Z
External ip config for winrm byon location
----
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118444720
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,55 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
+ machineConfig.put("address", machineConfig.get("winrm"));
+ machineConfig.remove("winrm");
} else {
- // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
- userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
- }
-
- // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
- port = userAndHostAndPort.getHostAndPort().getPort();
- if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
- String override = tcpPortMappings.get(port);
- HostAndPort hostAndPortOverride = HostAndPort.fromString(override);
- if (!hostAndPortOverride.hasPort()) {
- throw new IllegalArgumentException("Invalid portMapping ('"+override+"') for port "+port+" in "+specForErrMsg);
+ String winrm = (String) machineConfig.remove("winrm");
+
+ Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
+
+ checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
+
+ UserAndHostAndPort userAndHostAndPort;
+ String host;
+ int port;
+ if (ssh != null) {
+ userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ } else {
+ // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
+ userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
--- End diff --
Use a variable here to store the result of the logical expression, it will make it a lot easier to read.
The second clause should be `Boolean.parseBoolean(vals.get("winrm.useHttps").toString())`.
---
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] brooklyn-server issue #690: External ip config for winrm byon location
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on the issue:
https://github.com/apache/brooklyn-server/pull/690
retest this 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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118449716
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,55 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
+ machineConfig.put("address", machineConfig.get("winrm"));
+ machineConfig.remove("winrm");
} else {
- // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
- userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
- }
-
- // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
- port = userAndHostAndPort.getHostAndPort().getPort();
- if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
- String override = tcpPortMappings.get(port);
- HostAndPort hostAndPortOverride = HostAndPort.fromString(override);
- if (!hostAndPortOverride.hasPort()) {
- throw new IllegalArgumentException("Invalid portMapping ('"+override+"') for port "+port+" in "+specForErrMsg);
+ String winrm = (String) machineConfig.remove("winrm");
+
+ Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
+
+ checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
+
+ UserAndHostAndPort userAndHostAndPort;
+ String host;
+ int port;
+ if (ssh != null) {
+ userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ } else {
+ // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
+ userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
--- End diff --
Do the boolean expression in a separate variable for readability.
Second clause should be `Boolean.parseValue(vals.get("winrm.useHttps").toString())`.
---
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] brooklyn-server issue #690: External ip config for winrm byon location
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on the issue:
https://github.com/apache/brooklyn-server/pull/690
@geomacy yep, I'm currently working on it
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r117673212
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,54 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
--- End diff --
Looks good.
@iyovcheva what do you think about supporting external provider for ssh address as well. It will be more consistent.
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118441832
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WinRmMachineLocationExternalConfigYamlTest.java ---
@@ -89,9 +94,36 @@ public void testWindowsMachinesNoPasswordExternalProvider() throws Exception {
BasicApplication app = (BasicApplication) createAndStartApplication(yaml);
waitForApplicationTasks(app);
+ assertNull(RecordingWinRmTool.constructorProps.get(0).get(WinRmMachineLocation.USER.getName()));
assertNull(RecordingWinRmTool.constructorProps.get(0).get(WinRmMachineLocation.PASSWORD.getName()));
}
+ @Test()
+ public void testWindowsMachinesNoExternalIPProvider() throws Exception {
+ RecordingWinRmTool.constructorProps.clear();
+ final String yaml = Joiner.on("\n").join("location:",
+ " byon:",
+ " hosts:",
+ " - winrm: $brooklyn:external(\"inPlaceSupplier1\", \"ipEmpty\")",
+ " user: $brooklyn:external(\"inPlaceSupplier1\", \"byonUserEmpty\")",
+ " brooklyn.winrm.config.winrmToolClass: org.apache.brooklyn.util.core.internal.winrm.RecordingWinRmTool",
+ " password: $brooklyn:external(\"inPlaceSupplier1\", \"byonPasswordddd\")",
+ " osFamily: windows",
+ "services:",
+ "- type: org.apache.brooklyn.entity.software.base.VanillaWindowsProcess",
+ " brooklyn.config:",
+ " launch.command: echo launch",
+ " checkRunning.command: echo running");
+
+ try {
+ BasicApplication app = (BasicApplication) createAndStartApplication(yaml);
+ waitForApplicationTasks(app);
+ Asserts.shouldHaveFailedPreviously();
+ } catch (Exception e) {
+ Asserts.expectedFailureOfType(e, NullPointerException.class);
--- End diff --
It's bad if we are throwing null pointer exceptions - these should be caught lower in the code and at least turned into something more useful with a useful error message that you can then test in here. For example use `Preconditions.checkArgument` on whatever is throwing the NPE.
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118450024
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,54 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
--- End diff --
`!(machineConfig.get("winrm") instanceof String)` might be better as `machineConfig.get("winrm") instanceof DeferredSupplier`?
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r117683610
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,54 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
--- End diff --
@bostko I agree. Will need to test it.
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118446883
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,55 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
+ machineConfig.put("address", machineConfig.get("winrm"));
+ machineConfig.remove("winrm");
} else {
- // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
- userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
- }
-
- // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
- port = userAndHostAndPort.getHostAndPort().getPort();
- if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
- String override = tcpPortMappings.get(port);
- HostAndPort hostAndPortOverride = HostAndPort.fromString(override);
- if (!hostAndPortOverride.hasPort()) {
- throw new IllegalArgumentException("Invalid portMapping ('"+override+"') for port "+port+" in "+specForErrMsg);
+ String winrm = (String) machineConfig.remove("winrm");
+
+ Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
+
+ checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
+
+ UserAndHostAndPort userAndHostAndPort;
+ String host;
+ int port;
+ if (ssh != null) {
+ userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ } else {
+ // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
+ userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
}
- port = hostAndPortOverride.getPort();
- host = hostAndPortOverride.getHostText().trim();
- } else {
- host = userAndHostAndPort.getHostAndPort().getHostText().trim();
- }
-
- machineConfig.put("address", host);
- try {
- InetAddress.getByName(host);
- } catch (Exception e) {
- throw new IllegalArgumentException("Invalid host '"+host+"' specified in '"+specForErrMsg+"': "+e);
- }
- if (userAndHostAndPort.getUser() != null) {
- checkArgument(!vals.containsKey("user"), "Must not specify user twice for machine: %s", valSanitized);
- machineConfig.put("user", userAndHostAndPort.getUser());
- }
- if (userAndHostAndPort.getHostAndPort().hasPort()) {
- checkArgument(!vals.containsKey("port"), "Must not specify port twice for machine: %s", valSanitized);
- machineConfig.put("port", port);
+ // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
+ port = userAndHostAndPort.getHostAndPort().getPort();
+ if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
+ String override = tcpPortMappings.get(port);
+ HostAndPort hostAndPortOverride = HostAndPort.fromString(override);
+ if (!hostAndPortOverride.hasPort()) {
+ throw new IllegalArgumentException("Invalid portMapping ('"+override+"') for port "+port+" in "+specForErrMsg);
+ }
+ port = hostAndPortOverride.getPort();
+ host = hostAndPortOverride.getHostText().trim();
+ } else {
+ host = userAndHostAndPort.getHostAndPort().getHostText().trim();
+ }
+
+ machineConfig.put("address", host);
+ try {
+ InetAddress.getByName(host);
+ } catch (Exception e) {
--- End diff --
`UnknownHostException`
---
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] brooklyn-server issue #690: External ip config for winrm byon location
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/690
hi @iyovcheva just checking over brooklyn-server open PRs, what's the latest on this one? Do you want to update it as @neykov mentioned [above](https://github.com/apache/brooklyn-server/pull/690#discussion_r118445857)?
---
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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118445933
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,55 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
+ machineConfig.put("address", machineConfig.get("winrm"));
+ machineConfig.remove("winrm");
} else {
- // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
- userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
- }
-
- // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
- port = userAndHostAndPort.getHostAndPort().getPort();
- if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
- String override = tcpPortMappings.get(port);
- HostAndPort hostAndPortOverride = HostAndPort.fromString(override);
- if (!hostAndPortOverride.hasPort()) {
- throw new IllegalArgumentException("Invalid portMapping ('"+override+"') for port "+port+" in "+specForErrMsg);
+ String winrm = (String) machineConfig.remove("winrm");
+
+ Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
+
+ checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
+
+ UserAndHostAndPort userAndHostAndPort;
+ String host;
+ int port;
+ if (ssh != null) {
+ userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ } else {
+ // TODO set to null and rely on the MachineLocation. If not then make a dependency to WinRmMachineLocation and use its config key name.
+ userAndHostAndPort = parseUserAndHostAndPort(winrm, vals.get("winrm.useHttps") != null && (Boolean)vals.get("winrm.useHttps") ? 5986 : 5985);
}
- port = hostAndPortOverride.getPort();
- host = hostAndPortOverride.getHostText().trim();
- } else {
- host = userAndHostAndPort.getHostAndPort().getHostText().trim();
- }
-
- machineConfig.put("address", host);
- try {
- InetAddress.getByName(host);
- } catch (Exception e) {
- throw new IllegalArgumentException("Invalid host '"+host+"' specified in '"+specForErrMsg+"': "+e);
- }
- if (userAndHostAndPort.getUser() != null) {
- checkArgument(!vals.containsKey("user"), "Must not specify user twice for machine: %s", valSanitized);
- machineConfig.put("user", userAndHostAndPort.getUser());
- }
- if (userAndHostAndPort.getHostAndPort().hasPort()) {
- checkArgument(!vals.containsKey("port"), "Must not specify port twice for machine: %s", valSanitized);
- machineConfig.put("port", port);
+ // If there is a tcpPortMapping defined for the connection-port, then use that for ssh/winrm machine
+ port = userAndHostAndPort.getHostAndPort().getPort();
+ if (tcpPortMappings != null && tcpPortMappings.containsKey(port)) {
--- End diff --
I would do this `if` in a separate method, this one is long enough already!
---
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] brooklyn-server issue #690: External ip config for winrm byon location
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on the issue:
https://github.com/apache/brooklyn-server/pull/690
Failing with "cannot create temporary folders in parent /tmp after 1000 attempts" which is not related to the 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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r118445857
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,55 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
--- End diff --
@iyovcheva The changes here break the support for specifying `winrm` as `user@host:port`. Can you add it to the test cases.
On the other hand your approach is also interesting because it doesn't resolve the value here - at the time of deploying the blueprint. Obtaining the machine could happen much later in the lifecycle where the external values have already changed.
A proper solution would involve moving all of non-essential parsing to the machine location, leaving only minimal sanity checks 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] brooklyn-server pull request #690: External ip config for winrm byon locatio...
Posted by iyovcheva <gi...@git.apache.org>.
Github user iyovcheva commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/690#discussion_r117935379
--- Diff: core/src/main/java/org/apache/brooklyn/location/byon/ByonLocationResolver.java ---
@@ -152,49 +153,54 @@ protected ConfigBag extractConfig(Map<?,?> locationFlags, String spec, LocationR
String osFamily = (String) machineConfig.remove(OS_FAMILY.getName());
String ssh = (String) machineConfig.remove("ssh");
- String winrm = (String) machineConfig.remove("winrm");
- Map<Integer, String> tcpPortMappings = (Map<Integer, String>) machineConfig.get("tcpPortMappings");
-
- checkArgument(ssh != null ^ winrm != null, "Must specify exactly one of 'ssh' or 'winrm' for machine: %s", valSanitized);
-
- UserAndHostAndPort userAndHostAndPort;
- String host;
- int port;
- if (ssh != null) {
- userAndHostAndPort = parseUserAndHostAndPort(ssh, 22);
+ if (machineConfig.containsKey("winrm") && !(machineConfig.get("winrm") instanceof String)) {
--- End diff --
External config is not supported for ssh. If we implement it, it's good to go in separate 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] brooklyn-server issue #690: External ip config for winrm byon location
Posted by tbouron <gi...@git.apache.org>.
Github user tbouron commented on the issue:
https://github.com/apache/brooklyn-server/pull/690
retest this please
---