You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2015/12/25 14:03:11 UTC
[GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/1287
SecurityGroupRulesCmd code cleanup
Wrote a test and cleaned some duplicate code with the objective to evaluate the jenkins pull request process at builds.a.o
worthwhie to keep, IMHO.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack securityrules-cleanup
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1287.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 #1287
----
commit 2b38b2b6635a2b8818faad4764ea2a43825f827b
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-12-25T12:51:50Z
security rules test
commit da33dd870671d8c9fee44b1f0237e88aa51909cf
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-12-25T12:04:11Z
code cleanup
----
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-168988361
Is there a way to test this quickly or do you have any results?
---
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.
---
Re: [GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by Daan Hoogland <da...@gmail.com>.
On Sat, Jan 16, 2016 at 12:57 PM, rafaelweingartner <gi...@git.apache.org>
wrote:
> Github user rafaelweingartner commented on the pull request:
>
> https://github.com/apache/cloudstack/pull/1287#issuecomment-172196150
>
> @DaanHoogland,
> I reviewed your code and I have a few suggestions/questions.
>
> Are we really going to use final in all of our variables/attributes? I
> personally do not like to use final declarations, I use them only if
> needed; they tend to make extensions harder (sometimes).
>
Making the extension more consious is the purpose of making all final.
Harder is actually an aid for a dev in checking all places a var needs to
mutable. I do think we should make vars as final as possible.
>
> The following comments are about “SecurityGroupRulesCmd” class:
> At lines 73, 129, 140 and maybe others, why not initialize the
> ArrayList when you declare the attribute, this way it is always ready for
> use. I personally like to work this way, initializing List, Maps and others
> at the moment of variable declaration.
>
at those lines, the contents of the var might be replaced as well as
initialized.
>
> I would also suggest improving the java doc at method
> “stringifyRulesFor” to make it clear of its purpose and how it works.
>
I am with @mferreira on this. If comments are needed, what is actually
needed is improvement of the code. This javadoc was automatically created
and the method is private. I will remove the javadoc.
> The comment of method “compressCidr” should be converted to proper
> java doc style.
I will rename the method to something more descriptive instead.
> I would also use the "/" magical character as a constant.
>
will do.
> At line 206, I would suggest you extracting the code to a method and
> changing the contracted “IF” to a normal if structure, improving
> readability.
>
makes sense but I will have a look at a good solution.
>
> I also recommend changing the comment of “compressStringifiedRules”
> method to proper java doc style. If you do that, please extract the comment
> at lines 221 and 222 to the java doc.
>
I will have a second look at the comments but these are beyond the scope
of this PR.
>
> I recommend you removing the comment of line 251. If there is
> something to be said about that method, that should be done at the Java doc.
>
sure, but please at a PR yourself, this one is definately beyond the scope
of this PR, which is the removal of duplicate code to address a sonarqube
warning.
>
> The following comments are about “SecurityGroupRulesCmdTest” class.
> At the test if the “setUpBeforeClass” is not used, I suggest removing
> it. I think the best philosophy is to add code only when needed.
>
valid point, will do.
>
> Why does the class “SecurityGroupHttpClient” is being marked as
> completely changed? It seems that nothing has been changed there. Just some
> small adjusts at line 75 and 76
>
This is usually due to line endings. I am inclined to ignore this. The
class is small and the result is fine.
>
> I believe that is all. There are other suggestions to add some more
> Java doc and tests, but let's keep them to another PR in the near future.
>
test yes, javadoc only if the title of the non-private method can not be
made to be descriptive enough.
>
> ---
> 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.
> ---
>
--
Daan
[GitHub] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172196150
@DaanHoogland,
I reviewed your code and I have a few suggestions/questions.
Are we really going to use final in all of our variables/attributes? I personally do not like to use final declarations, I use them only if needed; they tend to make extensions harder (sometimes).
The following comments are about “SecurityGroupRulesCmd” class:
At lines 73, 129, 140 and maybe others, why not initialize the ArrayList when you declare the attribute, this way it is always ready for use. I personally like to work this way, initializing List, Maps and others at the moment of variable declaration.
I would also suggest improving the java doc at method “stringifyRulesFor” to make it clear of its purpose and how it works.
The comment of method “compressCidr” should be converted to proper java doc style. I would also use the "/" magical character as a constant.
At line 206, I would suggest you extracting the code to a method and changing the contracted “IF” to a normal if structure, improving readability.
I also recommend changing the comment of “compressStringifiedRules” method to proper java doc style. If you do that, please extract the comment at lines 221 and 222 to the java doc.
I recommend you removing the comment of line 251. If there is something to be said about that method, that should be done at the Java doc.
The following comments are about “SecurityGroupRulesCmdTest” class.
At the test if the “setUpBeforeClass” is not used, I suggest removing it. I think the best philosophy is to add code only when needed.
Why does the class “SecurityGroupHttpClient” is being marked as completely changed? It seems that nothing has been changed there. Just some small adjusts at line 75 and 76
I believe that is all. There are other suggestions to add some more Java doc and tests, but let's keep them to another PR in the near future.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172347935
@DaanHoogland, that is great.
Those points I lifted up were merely suggestions on how to improve the code a little bit more. Actually, in most PRs I review the code normally is ok, I just point out some aspects that can be improved.
I understand your point on using the final modifier. Since ACS is not a framework, I can agree with you here. I will start using them from now on.
We only diverge at the point regarding Java docs, and that is fine. I know that are some other development philosophies that do not like to use documentation, I understand their point. I also noticed that some people here do not like to use much Java doc and sometimes they confuse java doc with block comment.
I believe that all of Apache libraries/software I know are heavily documented, in a page that presents the use of the library/software and at the code. That is one of the reasons why I love apache Java libraries/software, because of their documentation, from which I can understand and use the software without needing to rely on someone else. IMO ACS should have more documentation on its code for two reasons.
First, it gives a purpose to the method or class. It forces people to think on what they will code into a method and stick to what was planned, instead of planning and creating code on the fly which in some cases lead to big chunks that are hard to maintain and refactor.
Second, Java docs facilitate to new developers to really understand the behavior and purpose of classes and methods. That also helps when developing new code; with proper documentation, I find it easier to uncover the right place to place a new method/function.
About documentation on private methods, I use them (write them sometimes). I also have used them (read) and find them quite useful in some components I use. Apache libraries such as Apache Commons use them. I understand that there are self-explanatory methods which do not need documentation. However, given the complexity of ACS, I believe that proper java doc would facilitate to new devs. I am not saying that we should start documenting everything. When I suggest the addition of a Java doc or a test case, it is merely a tentative to improve our code base. I see the process of creating Java docs the same as writing test cases, it takes time and a change in the mindset to get used to them.
Your code is great, I would just suggest some more improvement at the new method you created that is called “represent”, I believe the best name for the boolean variable should be something like, “shouldCompress” or” hasToCompress”. The name of the method does not sound descriptive to me, but if you guys are ok with it, I am too.
Giving all of that, it is a LGTM from 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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1287#discussion_r59231793
--- Diff: core/src/com/cloud/agent/api/SecurityGroupRulesCmd.java ---
@@ -200,52 +167,43 @@ public String getSecIpsString() {
return sb.toString();
}
+ public String stringifyRules() {
+ StringBuilder ruleBuilder = new StringBuilder();
+ stringifyRulesFor(getIngressRuleSet(), INGRESS_RULE, false, ruleBuilder);
+ stringifyRulesFor(getEgressRuleSet(), EGRESS_RULE, false, ruleBuilder);
+ return ruleBuilder.toString();
+ }
+
public String stringifyCompressedRules() {
StringBuilder ruleBuilder = new StringBuilder();
- for (SecurityGroupRulesCmd.IpPortAndProto ipPandP : getIngressRuleSet()) {
- ruleBuilder.append("I:").append(ipPandP.getProto()).append(":").append(ipPandP.getStartPort()).append(":").append(ipPandP.getEndPort()).append(":");
- for (String cidr : ipPandP.getAllowedCidrs()) {
- //convert cidrs in the form "a.b.c.d/e" to "hexvalue of 32bit ip/e"
- ruleBuilder.append(compressCidr(cidr)).append(",");
- }
- ruleBuilder.append("NEXT");
- ruleBuilder.append(" ");
- }
- for (SecurityGroupRulesCmd.IpPortAndProto ipPandP : getEgressRuleSet()) {
- ruleBuilder.append("E:").append(ipPandP.getProto()).append(":").append(ipPandP.getStartPort()).append(":").append(ipPandP.getEndPort()).append(":");
+ stringifyRulesFor(getEgressRuleSet(), INGRESS_RULE, true, ruleBuilder);
+ stringifyRulesFor(getEgressRuleSet(), EGRESS_RULE, true, ruleBuilder);
+ return ruleBuilder.toString();
+ }
+
+ /**
+ * @param ipPandPs
+ * @param gression
+ * @param compress
+ * @param ruleBuilder
+ */
+ void stringifyRulesFor(SecurityGroupRulesCmd.IpPortAndProto[] ipPandPs, String gression, boolean compress, StringBuilder ruleBuilder) {
--- End diff --
@DaanHoogland What is "gression" (google doesn't return any meaningful result)? Wouldn't "ruleType" be a better name here?
OR another way would be to use a boolean (isIngress) and inside use the appropriate string based on the flag.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-168992910
@wido I have not changed logic so am trusting the unit tests and wrote an extra. A run of integration tests might be required by some ;)
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-169134419
Based on the code I don't see anything dangerous though. It still compiles I see ;)
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by cristofolini <gi...@git.apache.org>.
Github user cristofolini commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172234191
@rafaelweingartner @DaanHoogland,
Personally, I don't see a problem using final declarations for those attributes. They all do seem to contain information that shouldn't be changed once instantiated.
Other than that observation I feel comfortable agreeing with all of @rafaelweingartner's suggestions. And besides adding a few more javadocs to the new methods and cleaning up the existing ones, I don't see anything lacking here. As the new release approaches, it would be fine to add these things at a later date.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-202238647
### ACS CI BVT Run
**Sumarry:**
Build Number 138
Hypervisor xenserver
NetworkType Advanced
Passed=106
Failed=0
Skipped=4
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso
**Passed test suits:**
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_guest_vlan_range.TestDedicateGuestVlanRange
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_volumes.TestCreateVolume
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172341007
@rafaelweingartner I gave you your way more then I intended to with respect to comments. Please have a look if this satisfies your need/requirements.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172194485
Based on the code yes, @DaanHoogland . Couldn't run any tests 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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-208431289
+1 based on code review and test results posted by @bvbharatk.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1287
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-167294361
force pushed squashed commits. not sure why SecurityGroupHttpClient got replaced completely, probably line endings.
Last few runs nothing strange in the jenkins job, so I think it is good to stick to this one.
@remibergsma @ustcweizhou can you verify this code?
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172354935
What I did not understand is that when I read I think, represent what? Into what? What format?
Reading the method call I know it is doing something with the Cidr, but I do not know what.
It can get the Cidr and represent it into a string of bits, it can also represent an Ipv4 Cidr into its correspondent version of Ipv6 or it can receive a Cidr as a string of bits and represent it into its corresponding version in Ipv4 or Ipv6. Then, there is the compression parameter (I liked the “withCompression” name), I do not understand what that means. What does it mean to compress a Cidr? Only if I look at the whole stack of methods I can understand what sis being done there.
I also agree with you that the java doc talk should be done elsewhere.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172353941
Thanks @rafaelweingartner. as for your last remark
```
Your code as always is great, I would just suggest some more improvement at the new method you created that is called “represent”, I believe the best name for the boolean variable should be something like, “shouldCompress” or” hasToCompress”. The name of the method does not sound descriptive to me, but if you guys are ok with it, I am too.
```
maybe ```withCompression``` would have been better then ```compressed```. I think the phrase ``` represent(cidr,compressed)``` expresses quite elegantly what it does however. Wouldn't you say so?
We should have the talk on javadocs some other time. I think javadoc tends to outdate.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172245992
@DaanHoogland Can't test it as we don't use security groups.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by koushik-das <gi...@git.apache.org>.
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1287#discussion_r59233185
--- Diff: core/src/com/cloud/agent/api/SecurityGroupRulesCmd.java ---
@@ -200,52 +167,43 @@ public String getSecIpsString() {
return sb.toString();
}
+ public String stringifyRules() {
+ StringBuilder ruleBuilder = new StringBuilder();
+ stringifyRulesFor(getIngressRuleSet(), INGRESS_RULE, false, ruleBuilder);
+ stringifyRulesFor(getEgressRuleSet(), EGRESS_RULE, false, ruleBuilder);
+ return ruleBuilder.toString();
+ }
+
public String stringifyCompressedRules() {
StringBuilder ruleBuilder = new StringBuilder();
- for (SecurityGroupRulesCmd.IpPortAndProto ipPandP : getIngressRuleSet()) {
- ruleBuilder.append("I:").append(ipPandP.getProto()).append(":").append(ipPandP.getStartPort()).append(":").append(ipPandP.getEndPort()).append(":");
- for (String cidr : ipPandP.getAllowedCidrs()) {
- //convert cidrs in the form "a.b.c.d/e" to "hexvalue of 32bit ip/e"
- ruleBuilder.append(compressCidr(cidr)).append(",");
- }
- ruleBuilder.append("NEXT");
- ruleBuilder.append(" ");
- }
- for (SecurityGroupRulesCmd.IpPortAndProto ipPandP : getEgressRuleSet()) {
- ruleBuilder.append("E:").append(ipPandP.getProto()).append(":").append(ipPandP.getStartPort()).append(":").append(ipPandP.getEndPort()).append(":");
+ stringifyRulesFor(getEgressRuleSet(), INGRESS_RULE, true, ruleBuilder);
+ stringifyRulesFor(getEgressRuleSet(), EGRESS_RULE, true, ruleBuilder);
+ return ruleBuilder.toString();
+ }
+
+ /**
+ * @param ipPandPs
+ * @param gression
+ * @param compress
+ * @param ruleBuilder
+ */
+ void stringifyRulesFor(SecurityGroupRulesCmd.IpPortAndProto[] ipPandPs, String gression, boolean compress, StringBuilder ruleBuilder) {
--- End diff --
@DaanHoogland My bad, was looking at the individual commits. Saw that this is already fixed.
---
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] cloudstack pull request: SecurityGroupRulesCmd code cleanup
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1287#issuecomment-172191645
@wido is that a LGTM
@schubergphilis ?
@rafaelweingartner or any of the other brazilian colleagues?
@shapeblue ?
---
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.
---