You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by kjduling <gi...@git.apache.org> on 2016/12/14 21:31:59 UTC
[GitHub] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
GitHub user kjduling opened a pull request:
https://github.com/apache/geode/pull/316
GEODE-2188: ExampleSecurityManager references SampleSecurityManager i\u2026
\u2026n javadoc
This corrects some output messages and javadoc entries after the ExampleSecurityManager was moved. It also points the old SampleSecurityManagerTest at ExampleSecurityManager.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kjduling/apache-geode feature/GEODE-2188
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode/pull/316.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 #316
----
commit 135c2c7d268f1b985fc07bac855bc4c2df680281
Author: Kevin J. Duling <kd...@pivotal.io>
Date: 2016-12-14T21:23:19Z
GEODE-2188: ExampleSecurityManager references SampleSecurityManager in javadoc
----
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by kjduling <gi...@git.apache.org>.
Github user kjduling commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r93109555
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
Originally, yes, they were in the same package. Then, after SampleSecurityManager was moved to ExampleSecurityManager and TestSecurityManger, the test was testing TestSecurityManager, which is silly. A test to test a test? By changing the test to point to ExampleSecurityManager, scope became an issue.
We could also just move the test to the same package as the example and remove the accessors and public scope on the inner classes. Either is fine with me.
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by kjduling <gi...@git.apache.org>.
Github user kjduling commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r92662419
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
I didn't add them, I just marked them public so that the Test could reach them. It didn't make sense to me for TestSecurityManagerTest to be testing TestSecurityManager. It makes slightly more sense to test the ExampleSecurityManager. The two security managers are identical code at this point (RE: GEODE-2092), but I'm sure that'll change in the near future as we build out Geode examples.
If we were to remove Roles and Users, we'd also remove about 95% of the code in ExampleSecurityManager, not leaving much example left. We should have a separate ticket to refactor the example. This one really is only supposed to address some Javadoc and System.out messages.
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r92888056
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
I meant those getters and setters inside class User and class Role. Those were not there before your change set unless I am missing something.....
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r93097202
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
why was it working before? Was it because it was in the same package? If your test code is testing ExampleSecurityManager, then better just leave it public and have the getter/setters there.
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode/pull/316
---
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] geode issue #316: GEODE-2188: ExampleSecurityManager references SampleSecuri...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on the issue:
https://github.com/apache/geode/pull/316
let's keep the example package to contain only examples.
On Mon, Dec 19, 2016 at 12:09 PM, Kevin Duling <no...@github.com>
wrote:
> *@kjduling* commented on this pull request.
> ------------------------------
>
> In geode-core/src/main/java/org/apache/geode/examples/security/
> ExampleSecurityManager.java <https://github.com/apache/geode/pull/316>:
>
> > List<ResourcePermission> permissions = new ArrayList<>();
> +
>
> Originally, yes, they were in the same package. Then, after
> SampleSecurityManager was moved to ExampleSecurityManager and
> TestSecurityManger, the test was testing TestSecurityManager, which is
> silly. A test to test a test? By changing the test to point to
> ExampleSecurityManager, scope became an issue.
>
> We could also just move the test to the same package as the example and
> remove the accessors and public scope on the inner classes. Either is fine
> with me.
>
> \u2014
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/geode/pull/316>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AO6fkmr1fpyCEz1IHZKAP1JDdo2We37Oks5rJuR2gaJpZM4LNb_H>
> .
>
--
Cheers
Jinmei
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by kjduling <gi...@git.apache.org>.
Github user kjduling commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r93077824
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
Oh, I see. Yes, because I made the object public, it's been a habit of mine to then make accessor methods for the protected fields. I'll remove them.
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by jinmeiliao <gi...@git.apache.org>.
Github user jinmeiliao commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r92544045
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
These methods are not used anywhere, why do we need to add them?
---
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] geode pull request #316: GEODE-2188: ExampleSecurityManager references Sampl...
Posted by kjduling <gi...@git.apache.org>.
Github user kjduling commented on a diff in the pull request:
https://github.com/apache/geode/pull/316#discussion_r93079596
--- Diff: geode-core/src/main/java/org/apache/geode/examples/security/ExampleSecurityManager.java ---
@@ -257,15 +257,65 @@ private void readUsers(final Map<String, User> rolesToUsers, final JsonNode node
return roleMap;
}
- static class Role {
+ public static class Role {
List<ResourcePermission> permissions = new ArrayList<>();
+
--- End diff --
They are actually needed by the test. I can either set the fields public, move the test to the same package, or leave the accessor methods. Which is preferred?
---
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.
---