from:"jujoramos"

[GitHub] geode pull request #664: Fix for GEODE-3292

2017-07-28 Thread jujoramos

GitHub user jujoramos opened a pull request:

https://github.com/apache/geode/pull/664

Fix for GEODE-3292

Embedded PULSE Connection Failure When jmx-manager-bind-address != localhost

- ManagementAgent sets the 'pulse.host' system property whenever a 
'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' 
system property, reverting to 'localhost' by default.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [X] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


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

    $ git pull https://github.com/jujoramos/geode feature/GEODE-3292

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

https://github.com/apache/geode/pull/664.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 #664


commit 0c3e15f619ddd08f02ba8e2a8818f22d308c8bde
Author: Juan Jose Ramos Cassella 
Date:   2017-07-28T09:55:20Z

GEODE-3292: Embedded PULSE Connection Failure When jmx-manager-bind-address 
!= localhost

- ManagementAgent sets the 'pulse.host' system property whenever a 
'jmx-manager-bind-address' is configured.
- PULSE gets the hostname of the local JMX Manager through the 'pulse.host' 
system property, reverting to 'localhost' by default.




---
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] geode issue #664: Fix for GEODE-3292

2017-08-01 Thread jujoramos

Github user jujoramos commented on the issue:

https://github.com/apache/geode/pull/664
  
Hey @jaredjstewart,

Thanks for looking at this.
I'll write the tests, no worries at all.
Cheers.


---
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] geode issue #664: Fix for GEODE-3292

2017-08-02 Thread jujoramos

Github user jujoramos commented on the issue:

https://github.com/apache/geode/pull/664
  
Hello @jaredjstewart,

Just pushed the requested changes.

- Renamed `PulseVerificationTest` to `PulseConnectivityTest`.
- Removed `loginWithIncorrectPassword` test method, it is the same as the 
one in `PulseSecurityTest`.
- Added integration tests to check the connectivity between PULSE and the 
local jmx-manager, both when the default and non-default bind-address is used.
- Added `PulseAppListenerTest` unit test to verify that the repository is 
correctly configured by the `PulseAppListener` when PULSE is running in 
embedded mode.

Cheers.


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071353
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.geode.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
--- End diff --

Yup, you're right, I didn't see the security manager within the rule and 
just looked at the method implementation. The login test doesn't seem to be 
functionality related to the connectivity test, that's why I removed it 
completely and aggregated the tests in a single class with two inner 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071365
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/PulseConnectivityTest.java
 ---
@@ -0,0 +1,110 @@
+/*
+ * 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.geode.tools.pulse;
+
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.net.InetAddress;
+import java.util.Properties;
+
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.experimental.runners.Enclosed;
+import org.junit.runner.RunWith;
+
+@RunWith(Enclosed.class)
+@Category(IntegrationTest.class)
+public class PulseConnectivityTest {
+  @ClassRule
+  public static LocatorStarterRule locator = new 
LocatorStarterRule().withJMXManager();
+
+  // Test Connectivity for Default Configuration
+  @Category(IntegrationTest.class)
+  public static class DefaultBindAddressTest {
+@Rule
+public EmbeddedPulseRule pulse = new EmbeddedPulseRule();
+
+@BeforeClass
+public static void beforeClass() throws Exception {
--- End diff --

True, but I'm not using `withAutoStart()` for a good reason: the two inner 
classes need to setup the locator differently (the `jmx-manager-bind-address` 
value changes). 
I thought it's easier to read a single functional test class annotated with 
`@RunWith(Enclosed.class)` internally split in different inner classes than 
write a new test class to test exactly the same but changing a single locator 
property.



---
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] geode pull request #664: Fix for GEODE-3292

2017-08-03 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131071385
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/ManagementAgent.java
 ---
@@ -270,6 +271,7 @@ private void startHttpService(boolean isServer) {
   }
 
   System.setProperty(PULSE_EMBEDDED_PROP, "true");
+  System.setProperty(PULSE_HOST_PROP, "" + 
config.getJmxManagerBindAddress());
--- End diff --

I'll rewrite the tests and push the changes again, thanks for the 
suggestions!.


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-04 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131367740
  
--- Diff: 
geode-pulse/src/test/java/org/apache/geode/tools/pulse/internal/PulseAppListenerTest.java
 ---
@@ -0,0 +1,91 @@
+/*
+ * 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.geode.tools.pulse.internal;
+
+import javax.servlet.ServletContextEvent;
+import static org.mockito.Mockito.*;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.contrib.java.lang.system.RestoreSystemProperties;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TestRule;
+
+import org.apache.geode.test.junit.categories.UnitTest;
+import org.apache.geode.tools.pulse.internal.data.PulseConstants;
+import org.apache.geode.tools.pulse.internal.data.Repository;
+
+@Category(UnitTest.class)
+public class PulseAppListenerTest {
+  private Repository repository;
+  private PulseAppListener appListener;
+
+  @Rule
+  public final TestRule restoreSystemProperties = new 
RestoreSystemProperties();
+
+  @Before
+  public void setUp() {
+repository = Repository.get();
+appListener = new PulseAppListener();
+  }
+
+  @Test
+  public void embeddedModeDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(false, repository.isUseSSLManager());
+Assert.assertEquals(false, repository.isUseSSLLocator());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_PORT, 
repository.getPort());
+Assert.assertEquals(PulseConstants.GEMFIRE_DEFAULT_HOST, 
repository.getHost());
+
+  }
+
+  @Test
+  public void 
embeddedModeNonDefaultPropertiesRepositoryInitializationTest() {
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_EMBEDDED, 
"true");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_PORT, "");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_HOST, 
"nonDefaultBindAddress");
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_MANAGER,
+Boolean.TRUE.toString());
+System.setProperty(PulseConstants.SYSTEM_PROPERTY_PULSE_USESSL_LOCATOR,
+Boolean.TRUE.toString());
+
+appListener.contextInitialized(mock(ServletContextEvent.class));
+
+Assert.assertEquals(false, repository.getJmxUseLocator());
+Assert.assertEquals(true, repository.isUseSSLManager());
+Assert.assertEquals(true, repository.isUseSSLLocator());
+Assert.assertEquals("", repository.getPort());
+Assert.assertEquals("nonDefaultBindAddress", repository.getHost());
+  }
+
+  @After
+  public void tearDown() {
+if (repository != null) {
--- End diff --

Yes, I thought about using the existing rule but it implies adding a 
dependency from `geode-web` to `geode-assembly` as the rule is defined there, 
or duplicating the rule source code, that's why I didn't do it. Which approach 
would be best?.


---
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] geode pull request #664: Fix for GEODE-3292

2017-08-04 Thread jujoramos

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

https://github.com/apache/geode/pull/664#discussion_r131378087
  
--- Diff: 
geode-assembly/src/test/java/org/apache/geode/tools/pulse/AbstractPulseConnectivityTest.java
 ---
@@ -15,26 +15,31 @@
 
 package org.apache.geode.tools.pulse;
 
+import static 
org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_BIND_ADDRESS;
 import static org.assertj.core.api.Assertions.assertThat;
 
-import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
-import org.apache.geode.test.dunit.rules.HttpClientRule;
-import org.apache.geode.test.dunit.rules.LocatorStarterRule;
-import org.apache.geode.test.junit.categories.IntegrationTest;
-import org.apache.geode.tools.pulse.internal.data.Cluster;
+import java.io.File;
+import java.net.InetAddress;
+import java.util.Properties;
+
 import org.apache.http.HttpResponse;
 import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
+import org.junit.BeforeClass;
 
+import org.apache.geode.test.dunit.rules.EmbeddedPulseRule;
+import org.apache.geode.test.dunit.rules.HttpClientRule;
+import org.apache.geode.test.dunit.rules.LocatorStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.tools.pulse.internal.data.Cluster;
 
-@Category(IntegrationTest.class)
-public class PulseVerificationTest {
+public abstract class AbstractPulseConnectivityTest {
--- End diff --

Thanks for the tips, I couldn't find anything related to this intention 
within 
[Writing 
Tests](https://cwiki.apache.org/confluence/display/GEODE/Writing+tests) but I'm 
okay anyway.
I'll give it one more try in my next commit later today, change things as 
you 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode issue #664: Fix for GEODE-3292

2017-08-09 Thread jujoramos

Github user jujoramos commented on the issue:

https://github.com/apache/geode/pull/664
  
@jinmeiliao: thanks for reviewing this :-).
One more quick question and I'll leave you alone: do I need to change the 
status of [GEODE-3292](https://issues.apache.org/jira/browse/GEODE-3292) to 
`RESOLVED`?. Or that's done when the pull request is merged into `develop`?. 
Couldn't find a clear answer from [Code 
Contributions](https://cwiki.apache.org/confluence/display/GEODE/Code+contributions)
 and [Developer 
Workflow](https://cwiki.apache.org/confluence/display/GEODE/Developer+Workflow).
Cheers.


---
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] geode pull request #664: Fix for GEODE-3292

[GitHub] geode issue #664: Fix for GEODE-3292

[GitHub] geode issue #664: Fix for GEODE-3292

[GitHub] geode pull request #664: Fix for GEODE-3292

[GitHub] geode pull request #664: Fix for GEODE-3292

[GitHub] geode pull request #664: Fix for GEODE-3292

[GitHub] geode pull request #664: Fix for GEODE-3292

[GitHub] geode pull request #664: Fix for GEODE-3292

[GitHub] geode issue #664: Fix for GEODE-3292

9 matches

Site Navigation

Mail list logo

Footer information