[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/brooklyn-server/pull/680


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[nakomis]

Github user nakomis commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116485510
  
--- Diff: 
locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocationStubbedTest.java
 ---
@@ -118,4 +123,17 @@ public void testWinrmConfigPassedToMachine() throws 
Exception {
 
assertEquals(machine.config().get(WinRmMachineLocation.COPY_FILE_CHUNK_SIZE_BYTES),
 Integer.valueOf(123));
 assertEquals(machine.config().get(WinRmTool.PROP_EXEC_TRIES), 
Integer.valueOf(456));
 }
+
+@Test
+public void testNodeSetupCustomizer() throws Exception {
+final String testMetadata = "test-metadata";
+obtainMachine(ImmutableMap.of(JCLOUDS_LOCATION_CUSTOMIZERS, 
ImmutableList.of(new BasicJcloudsLocationCustomizer(){
+@Override
+public void customize(JcloudsLocation location, NodeMetadata 
node, ConfigBag setup) {
+setup.configure(ADDITIONAL_CONNECTION_METADATA, 
testMetadata);
--- End diff --

I've added a null check for `location`, but I'm not sure what else to 
assert 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472840
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/internal/ssh/SshTool.java ---
@@ -86,6 +86,8 @@
 public static final ConfigKey PROP_LAST_MODIFICATION_DATE = 
newConfigKey("lastModificationDate", "Last-modification-date to be set on files 
copied/created (should be UTC/1000, ie seconds since 1970; default 0 usually 
means current)", 0L);
 public static final ConfigKey PROP_LAST_ACCESS_DATE = 
newConfigKey("lastAccessDate", "Last-access-date to be set on files 
copied/created (should be UTC/1000, ie seconds since 1970; default 0 usually 
means lastModificationDate)", 0L);
 public static final ConfigKey PROP_OWNER_UID = 
newConfigKey("ownerUid", "Default owner UID (not username) for files created on 
remote machine; default is unset", -1);
+
+ConfigKey ADDITIONAL_CONNECTION_METADATA = 
newStringConfigKey("additional.connection.metadata", "Can be used by a location 
customizer during provisioning to provide additional information, which in turn 
can be consumed by a custom ssh tool");
--- End diff --

Split onto multiple lines.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472348
  
--- Diff: 
locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
 ---
@@ -751,6 +751,10 @@ protected MachineLocation obtainOnce(ConfigBag setup) 
throws NoMachinesAvailable
 machineCreationSemaphore.release();
 }
 
+for (JcloudsLocationCustomizer customizer : customizers) {
+customizer.customize(this, node, setup);
--- End diff --

I'd move this to just after the `if (null == null)` 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472751
  
--- Diff: 
software/winrm/src/main/java/org/apache/brooklyn/util/core/internal/winrm/WinRmTool.java
 ---
@@ -86,6 +86,8 @@
 "Size of file chunks (in bytes) to be used when copying a file 
to the remote server", 
 1024);
 
+ConfigKey ADDITIONAL_CONNECTION_METADATA = 
newStringConfigKey("additional.connection.metadata", "Can be used by a location 
customizer during provisioning to provide additional information, which in turn 
can be consumed by a custom ssh tool");
--- End diff --

Personal preference for splitting onto multiple lines when it's this 
crazy-long a line. Otherwise, it's hard to tell if there's a default value or 
anything like that lurking at the very end. I'd go for one line per arg.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472215
  
--- Diff: 
locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
 ---
@@ -751,6 +751,10 @@ protected MachineLocation obtainOnce(ConfigBag setup) 
throws NoMachinesAvailable
 machineCreationSemaphore.release();
 }
 
+for (JcloudsLocationCustomizer customizer : customizers) {
+customizer.customize(this, node, setup);
--- End diff --

Looks like this should be moved down slightly - we don't set `node` until 
just after this, so we'll always be passing `null` to the customizer.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472841
  
--- Diff: 
software/winrm/src/main/java/org/apache/brooklyn/util/core/internal/winrm/WinRmTool.java
 ---
@@ -86,6 +86,8 @@
 "Size of file chunks (in bytes) to be used when copying a file 
to the remote server", 
 1024);
 
+ConfigKey ADDITIONAL_CONNECTION_METADATA = 
newStringConfigKey("additional.connection.metadata", "Can be used by a location 
customizer during provisioning to provide additional information, which in turn 
can be consumed by a custom ssh tool");
--- End diff --

Says "custom ssh tool".


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116473296
  
--- Diff: 
software/winrm/src/main/java/org/apache/brooklyn/util/core/internal/winrm/WinRmTool.java
 ---
@@ -86,6 +86,8 @@
 "Size of file chunks (in bytes) to be used when copying a file 
to the remote server", 
 1024);
 
+ConfigKey ADDITIONAL_CONNECTION_METADATA = 
newStringConfigKey("additional.connection.metadata", "Can be used by a location 
customizer during provisioning to provide additional information, which in turn 
can be consumed by a custom ssh tool");
--- End diff --

Perhaps word as something like "Can be used to pass additional custom data 
to the `WinrmTool`, which is especially useful if writing a bespoke tool 
implementation."

Your description is accurate for your use-case, but really it's not 
specific to the customizer - it could be passed in as normal location config or 
entity `provisioning.properties`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/680#discussion_r116472546
  
--- Diff: 
locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsSshMachineLocationStubbedTest.java
 ---
@@ -118,4 +123,17 @@ public void testWinrmConfigPassedToMachine() throws 
Exception {
 
assertEquals(machine.config().get(WinRmMachineLocation.COPY_FILE_CHUNK_SIZE_BYTES),
 Integer.valueOf(123));
 assertEquals(machine.config().get(WinRmTool.PROP_EXEC_TRIES), 
Integer.valueOf(456));
 }
+
+@Test
+public void testNodeSetupCustomizer() throws Exception {
+final String testMetadata = "test-metadata";
+obtainMachine(ImmutableMap.of(JCLOUDS_LOCATION_CUSTOMIZERS, 
ImmutableList.of(new BasicJcloudsLocationCustomizer(){
+@Override
+public void customize(JcloudsLocation location, NodeMetadata 
node, ConfigBag setup) {
+setup.configure(ADDITIONAL_CONNECTION_METADATA, 
testMetadata);
--- End diff --

Let's also assert that `node` is not null (given it will be, based on the 
bug I spotted (I believe)).
And might as well also assert that `location` is equal to the object we 
expect.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #680: Adds JcloudsLocationCustomizer hook to al...

[nakomis]

GitHub user nakomis opened a pull request:

https://github.com/apache/brooklyn-server/pull/680

Adds JcloudsLocationCustomizer hook to allow node / config configuration



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nakomis/brooklyn-server 
add-nodemetdata-customization-hook

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/brooklyn-server/pull/680.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 #680


commit 320c29e06dca6c2e99cc79a8b373ba1f914a8a3d
Author: Martin Harris 
Date:   2017-05-15T09:34:32Z

Adds JcloudsLocationCustomizer hook to allow node / config configuration




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---