Fixed the issue osc-core issue #543 #621

bmmukund · 2017-12-12T10:27:57Z

Description

Modified the Error message to be more appropriate when deleting a DA without deleting the SFC.

Motivation and Context

Error message should be more suggestive - e.g. DA cannot be deleted unless the SFC is deleted
#543

How Has This Been Tested?

Tested manually.

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the Code style guidelines of this project
My unit test follow the Unit test guidelines
Unit test coverage percentage is maintained(increased/remains the same but not decreased)

bmmukund · 2017-12-12T10:30:26Z

@sudhirappaji,
Please review my code changes.

sudhirappaji · 2017-12-12T16:44:05Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

@@ -53,6 +60,10 @@ public DistributedAppliance validateAndLoad(BaseDeleteRequest request) throws Ex
                            da.getId()));
        }

+        // DE5296->Error message to be more appropriate when deleting a DA without


remove comments related to the issue - DES296.

Thank you for your comments . I removed the comments and will be updated in next review request.

sudhirappaji · 2017-12-12T16:49:08Z

osc-server/src/main/java/org/osc/core/broker/service/persistence/VirtualSystemEntityMgr.java

@@ -185,7 +185,6 @@ public static synchronized Long generateUniqueTag(EntityManager em, VirtualSyste
    }

    public static boolean isProtectingWorkload(VirtualSystem vs) {
-        return !vs.getServiceFunctionChains().isEmpty() ||
-                CollectionUtils.emptyIfNull(vs.getDeploymentSpecs()).stream().anyMatch(ds -> DeploymentSpecEntityMgr.isProtectingWorkload(ds));
+        return CollectionUtils.emptyIfNull(vs.getDeploymentSpecs()).stream().anyMatch(ds -> DeploymentSpecEntityMgr.isProtectingWorkload(ds));


getServiceFunctionChains().isEmpty() should not be removed.

Its required to remove this check because getServiceFunctionChains().isEmpty() will not separate the SFC call and would not be able to throw the appropriate error message.

sudhirappaji · 2017-12-12T16:54:10Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

@@ -62,4 +73,26 @@ public DistributedAppliance validateAndLoad(BaseDeleteRequest request) throws Ex

        return da;
    }
+
+    private void validateDAReferenceToSfcAndSecurityGroup(EntityManager em, DistributedAppliance da) throws Exception {


Please apply the eclipse settings for code style.

Thank you for your review comments . I will use eclipse code style before putting next review request.

sudhirappaji · 2017-12-12T16:56:33Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+    private void validateDAReferenceToSfcAndSecurityGroup(EntityManager em, DistributedAppliance da) throws Exception {
+		for (VirtualSystem vs : da.getVirtualSystems()) {
+			if (!vs.getServiceFunctionChains().isEmpty()) {
+				// DE5296-->Look if these SFCs are binded to any of the SG of same DA


remove the comments

Thank you for your review comments . I removed it.

sudhirappaji · 2017-12-12T17:08:00Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+				// DE5296-->Look if these SFCs are binded to any of the SG of same DA
+				for (ServiceFunctionChain sfc : vs.getServiceFunctionChains()) {
+					List<SecurityGroup> sgList = SecurityGroupEntityMgr.listSecurityGroupsBySfcId(em, sfc.getId());
+					String sgNames = sgList.stream().filter(sg -> !sg.getMarkedForDeletion()).map(sg -> sg.getName())


why are we not looking for marked for deletion?

Thank you for your review comments .I made changes.

sudhirappaji · 2017-12-12T17:10:17Z

@bmmukund , make sure coding guidelines are checked and followed and also unit test guidelines are checked.

emanoelxavier · 2017-12-13T02:07:35Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

@@ -53,6 +60,10 @@ public DistributedAppliance validateAndLoad(BaseDeleteRequest request) throws Ex
                            da.getId()));
        }

+        // DE5296->Error message to be more appropriate when deleting a DA without
+        // deleting the SFC


Does not look like the line below is deleting, pls remote this comment.

Thank you for your review comments .I made changes

emanoelxavier · 2017-12-13T02:16:44Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+			if (!vs.getServiceFunctionChains().isEmpty()) {
+				// DE5296-->Look if these SFCs are binded to any of the SG of same DA
+				for (ServiceFunctionChain sfc : vs.getServiceFunctionChains()) {
+					List<SecurityGroup> sgList = SecurityGroupEntityMgr.listSecurityGroupsBySfcId(em, sfc.getId());


Add this to a private method to improve readability: validateSfcReferenceToSecurityGroup. This combination of private methods will make the overall intended validation clearer

Thank you for your review comments .I made changes as suggested.

emanoelxavier · 2017-12-13T02:19:06Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+
+    private void validateDAReferenceToSfcAndSecurityGroup(EntityManager em, DistributedAppliance da) throws Exception {
+		for (VirtualSystem vs : da.getVirtualSystems()) {
+			if (!vs.getServiceFunctionChains().isEmpty()) {


move this to a private method named validateVSReferenceToSfcAndSecurityGroup. In that method you can return immediately if the getSFCs is empty. This will reduce if-nesting and improve readability.

Thank you for your review comments .I moved it into new suggested method.

bmmukund · 2017-12-18T12:10:57Z

@emanoelxavier, @sudhirappaji,
Please review the changes.

emanoelxavier · 2017-12-18T16:43:44Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+	private void validateVSReferenceToSfcAndSecurityGroup(EntityManager em, DistributedAppliance da, VirtualSystem vs)
+			throws Exception {
+
+		if (!getSFCs(vs)) {


This is not improving the readability. It is also not reducing nesting. Instead simply start this method with if vs.getsfcs == null || vs.getsfcs.isempty then return immediately.

emanoelxavier · 2017-12-18T16:44:05Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+		return null;
+	}
+
+	private boolean getSFCs(VirtualSystem vs) {


Please remove this method, see the other comment.

emanoelxavier · 2017-12-18T16:50:25Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+			throws Exception {
+
+		if (!getSFCs(vs)) {
+			if (validateSfcReferenceToSecurityGroup(em, da, vs) != null) {


This pattern is very strange. A method or function returning an exception is a code smell and should always be avoided. Exceptions are usually meant to be thrown not returned. Especially here where you check to see if it returns and exception and if so you call the same method again but then you throw. instead simply make the method validateSfcReferenceToSG throw and call it for each sfc in the VS looping through the sfcs . You also should not need to pass the VS as a parameter for this method at all since you would have the loop of ln 93 on this method instead. The pattern is... if empty return, if not loop through the sfcs in the VS, for each SFC call validateSfcRErferencetoSG which does not take a VS and does not return anything.

arvindn05 · 2017-12-21T20:39:19Z

@bmmukund travis build needs to succeed so folks can continue reviewing. Also make sure your branch is upto date with master.
looks like below test is failing
testValidateAndLoad_WhenChainedToSfcDeleteRequest_ThrowsValidationException(org.osc.core.broker.service.request.DeleteDistributedApplianceRequestValidatorTest)

codecov · 2017-12-27T09:27:09Z

Codecov Report

Merging #621 into master will increase coverage by 0.01%.
The diff coverage is 53.84%.

@@             Coverage Diff              @@
##             master     #621      +/-   ##
============================================
+ Coverage     26.49%   26.51%   +0.01%     
- Complexity     1614     1617       +3     
============================================
  Files           501      501              
  Lines         20089    20113      +24     
  Branches       2380     2383       +3     
============================================
+ Hits           5323     5333      +10     
- Misses        14358    14368      +10     
- Partials        408      412       +4

Impacted Files	Coverage Δ	Complexity Δ
...er/service/persistence/VirtualSystemEntityMgr.java	`52.7% <100%> (+1.35%)`	`8 <2> (-1)`	⬇️
...or/DeleteDistributedApplianceRequestValidator.java	`70% <50%> (-30%)`	`13 <6> (+6)`
...ice/persistence/DistributedApplianceEntityMgr.java	`37.31% <0%> (-1.5%)`	`4% <0%> (-1%)`
...ver/src/main/java/org/osc/core/broker/job/Job.java	`68.37% <0%> (-0.61%)`	`69% <0%> (-1%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05d9339...1512249. Read the comment docs.

bmmukund · 2017-12-27T09:29:34Z

@arvindn05 ,
Please review the code changes which i have done for the unit test cases.

arvindn05 · 2017-12-27T16:50:16Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+				.collect(Collectors.joining(", "));
+		if (!sgNames.isEmpty()) {
+			throw new VmidcBrokerValidationException(String.format(
+					"Cannot delete deployment appliance with name '%s' which is currently referencing Service Function Chain '%s' and binded to a SecurityGroup(s) '%s'",


distributed appliance

arvindn05 · 2017-12-27T16:58:02Z

osc-server/src/main/java/org/osc/core/broker/service/persistence/VirtualSystemEntityMgr.java

@@ -185,7 +185,7 @@ public static synchronized Long generateUniqueTag(EntityManager em, VirtualSyste
    }

    public static boolean isProtectingWorkload(VirtualSystem vs) {


can you add javadoc to these methods to make it clear what they consider protected workload?

org.osc.core.broker.service.persistence.DistributedApplianceEntityMgr.isProtectingWorkload(DistributedAppliance)
org.osc.core.broker.service.persistence.VirtualSystemEntityMgr.isProtectingWorkload(VirtualSystem)
org.osc.core.broker.service.persistence.DeploymentSpecEntityMgr.isProtectingWorkload(DeploymentSpec)

please address this comment and add javadco.

emanoelxavier · 2017-12-28T02:27:24Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+		for (ServiceFunctionChain sfc : vs.getServiceFunctionChains()) {
+			validateSfcReferenceToSecurityGroup(em, da, sfc);
+		}
+


emanoelxavier · 2017-12-28T02:27:58Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+
+    private void validateVSReferenceToSfcAndSecurityGroup(EntityManager em, DistributedAppliance da, VirtualSystem vs)
+			throws Exception {
+		if (vs.getServiceFunctionChains() == null || vs.getServiceFunctionChains().isEmpty()) {


isempty is not needed here, if empty the loop below simply wont execute.

emanoelxavier · 2017-12-28T02:33:04Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+
+	}
+
+	private void validateSfcReferenceToSecurityGroup(EntityManager em, DistributedAppliance da,


It is a bit strange to have the da as a parameter here just to have the name in the exception message. The business logic of this method does not need a DA. Instead of passing the da it would be better to throw the exception without the da name in the message. On the method validateDAReferenceToSfcAndSecurityGroup (which a;ready has the DA) you can catch the exception, add the da name on the message and throw a new exception of the same type. This will keep the messages on the methods they belong to and avoid extra unnecessary parameters. In general the less parameters on a method the better,

emanoelxavier · 2017-12-28T02:46:01Z

...java/org/osc/core/broker/service/request/DeleteDistributedApplianceRequestValidatorTest.java

@@ -56,6 +67,9 @@
    @Mock
    EntityManager em;

+    @Mock


As per our unit test guidelines https://github.com/opensecuritycontroller/community/blob/master/development/code-guidelines/unit_test_guidelines.md *EntityManagers should not be mocked. There are two approaches we use to deal with database entities/dependencies in our unit tests:

Mocking the EntityManager (em.find): This works if the class under test depends simply on find by id or save, update, etc. This was the case for this class prior to your change, thus we were mocking em.find and that was enough here.

Using an in-memory db. This is needed if the class under test depends on more complex queries, typically using query builders. This is the case now for this class after your changes, listSecurityGroupsBySfcId uses a query builder. So this is the option you should choose to implement these unit tests. We have many unit tests using in memory db, for instance AgentRegisterServiceRequestValidatorTest. Please take a look at those to see how that works and you can mimic the same approach here.

@bmmukund looks like none of my comments on this file have been addressed.

emanoelxavier · 2017-12-28T03:16:28Z

...java/org/osc/core/broker/service/request/DeleteDistributedApplianceRequestValidatorTest.java

@@ -150,6 +165,63 @@ public void testValidateAndLoad_WhenChainedToSfcDeleteRequest_ThrowsValidationEx
        this.validator.validateAndLoad(createRequest(VALID_ID_WITH_SFC, false));
    }

+    @Test
+    public void testValidateSfcToSecurityGroup_WhenDADeleteRequest_ThrowsValidationException() throws Exception {


Please review the test naming convetion in the unit test guidelines. https://github.com/opensecuritycontroller/community/blob/master/development/code-guidelines/unit_test_guidelines.md#test-method-name

@bmmukund it seems you missed this comment.

emanoelxavier · 2017-12-28T03:16:40Z

...java/org/osc/core/broker/service/request/DeleteDistributedApplianceRequestValidatorTest.java

+    }
+
+   @Test
+    public void testValidateDaToSfc_WhenDADeleteRequest_ThrowsValidationException() throws Exception {


https://github.com/opensecuritycontroller/community/blob/master/development/code-guidelines/unit_test_guidelines.md#test-method-name

Modified and added test cases. Added back to original intialization Made the changes according to the review comments. Did required changes for unit test case. Did the changes as per review comments.

bmmukund · 2017-12-29T07:46:03Z

@emanoelxavier ,Please check if this be approved, so that Sudhir can go ahead and merge.

sudhirappaji · 2017-12-29T08:00:52Z

@bmmukund , I notice that commits (for while incorporating the review comments in PR) are squashed. This should not be done and finally before the merger to master, this shall be squashed by the approver/gate keeper. Otherwise history is lost while reviewing PR. Please take note of it.

arvindn05 · 2017-12-29T16:43:40Z

osc-server/src/main/java/org/osc/core/broker/service/persistence/VirtualSystemEntityMgr.java

@@ -185,7 +185,7 @@ public static synchronized Long generateUniqueTag(EntityManager em, VirtualSyste
    }

    public static boolean isProtectingWorkload(VirtualSystem vs) {


please address this comment and add javadco.

arvindn05 · 2017-12-29T16:50:02Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+            try {
+                validateVSReferenceToSfcAndSecurityGroup(em, vs);
+            } catch (VmidcBrokerValidationException ex) {
+                throw new VmidcBrokerValidationException(ex.getMessage() + " with name " + da.getName());


this will be a user facing message...we should format this better.

The message will show
Cannot delete distributed appliance which is currently referencing Service Function Chain 'foosfc' and binded to a SecurityGroup(s) 'foosg' with name 'fooda'

I would just avoid this catch block or format the message like below

`Cannot delete distributed appliance 'fooda' which is currently referencing Service Function Chain 'foosfc' and binded to a SecurityGroup(s) 'foosg'.

+1 for the suggested message. The main purpose of the catch is to avoid passing the da as a param just to have it in the message, makes the signature of the method validateVSReferenceToSfcAndSecurityGroup harder to understand.

emanoelxavier · 2018-01-03T00:28:54Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+            try {
+                validateVSReferenceToSfcAndSecurityGroup(em, vs);
+            } catch (VmidcBrokerValidationException ex) {
+                throw new VmidcBrokerValidationException(ex.getMessage() + " with name " + da.getName());


+1 for the suggested message. The main purpose of the catch is to avoid passing the da as a param just to have it in the message, makes the signature of the method validateVSReferenceToSfcAndSecurityGroup harder to understand.

bmmukund · 2018-01-12T12:41:56Z

@emanoelxavier,@arvindn05,@sudhirappaji,
Please review the incorporated changes for the review comments.

sudhirappaji · 2018-01-12T13:41:35Z

@bmmukund , why did you close this PR? This is yet to be approved.
After the approval, this shall be merged with the master. Please reopen the PR and allow the reviewers to review and approve.

emanoelxavier · 2018-01-12T16:25:05Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+            try {
+                validateVSReferenceToSfcAndSecurityGroup(em, vs);
+            } catch (VmidcBrokerValidationException ex) {
+                throw new VmidcBrokerValidationException(da.getName() + " " + ex.getMessage());


This should be "Cannot delete the DA {da.getName}," + ex.getMessage

emanoelxavier · 2018-01-12T16:29:42Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+                .collect(Collectors.joining(", "));
+        if (!sgNames.isEmpty()) {
+            throw new VmidcBrokerValidationException(String.format(
+                    "distributed appliance cannot be deleted which is currently referencing Service Function Chain '%s' and binded to a SecurityGroup(s) '%s'",


I believe you missed my comment before... once again there is nothing in this method about DA. The message should not have any comment about DA. It should be something like "The Service Function Chain sfc_name is referenced by the Security Group sg_name" . Also avoid using type names in the messages: SecurityGroup -> Security Group, this is also consistent with your use of Service Function Chain (rather than SFC or ServiceFunctionChain).

emanoelxavier · 2018-01-12T16:29:52Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+                    sfc.getName(), sgNames));
+        } else if (!sfc.getMarkedForDeletion() && sfc != null) {
+            throw new VmidcBrokerValidationException(String.format(
+                    "distributed appliance cannot be deleted which is currently referencing Service Function Chain '%s'",


same comment here.

emanoelxavier · 2018-01-12T16:31:13Z

...java/org/osc/core/broker/service/request/DeleteDistributedApplianceRequestValidatorTest.java


+@RunWith(PowerMockRunner.class)


Please add a TODO here explaining this is a workaround to justify the usage of powermock. The better solution should be refactoring this test class to use an in mem db.

emanoelxavier · 2018-01-12T16:34:46Z

...java/org/osc/core/broker/service/request/DeleteDistributedApplianceRequestValidatorTest.java

@@ -150,6 +162,65 @@ public void testValidateAndLoad_WhenChainedToSfcDeleteRequest_ThrowsValidationEx
        this.validator.validateAndLoad(createRequest(VALID_ID_WITH_SFC, false));
    }

+    @Test
+    public void testValidateDAReferenceToSfcAndSecurityGroup_WhenChainedToSfcAndSgDeleteRequest_ThrowsValidationException()


This test name is still not consistent with the guidelines. Please review all the details on the guidelines with attention and let me know if you have any questions. Test names should be testMethodUnderTest_... in this case testValidateAndLoad... just like the other methods in this class. Also please your TODO around these methods as well (the same TODO i mentioned above). Also ensure to track this debt with an issue on GitHub, the correct way of doing this is refactoring this test class to use an in mem db.

bmmukund · 2018-01-12T22:16:48Z

@emanoelxavier ,
I have done the required changes.Please review and let me know if any changes needs to be done.

emanoelxavier · 2018-01-12T22:23:13Z

osc-server/src/main/java/org/osc/core/broker/service/persistence/VirtualSystemEntityMgr.java

@@ -184,8 +184,12 @@ public static synchronized Long generateUniqueTag(EntityManager em, VirtualSyste
        return prevVal;
    }

+	/**


Please delete this javadoc before merging.

emanoelxavier · 2018-01-12T22:24:59Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+            try {
+                validateVSReferenceToSfcAndSecurityGroup(em, vs);
+            } catch (VmidcBrokerValidationException ex) {
+                throw new VmidcBrokerValidationException(String.format("Cannot delete the distributed appliance '%s' reason:",da.getName())+" "+ex.getMessage());


"Cannot delete the distributed appliance '%s' reason: ", da.getName()) + ex.getMessage()) Please attempt to the spaces and details.

emanoelxavier · 2018-01-12T22:25:49Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+        String sgNames = sgList.stream().filter(sg -> !sg.getMarkedForDeletion()).map(sg -> sg.getName())
+                .collect(Collectors.joining(", "));
+        if (!sgNames.isEmpty()) {
+            throw new VmidcBrokerValidationException(String.format("Service Function Chain '%s' and binded to a Security Group(s) '%s'",


and binded -> is binded. Please do a detailed cursory read of the code.

emanoelxavier · 2018-01-12T22:27:36Z

...n/java/org/osc/core/broker/service/validator/DeleteDistributedApplianceRequestValidator.java

+            throw new VmidcBrokerValidationException(String.format("Service Function Chain '%s' and binded to a Security Group(s) '%s'",
+                    sfc.getName(), sgNames));
+        } else if (!sfc.getMarkedForDeletion() && sfc != null) {
+            throw new VmidcBrokerValidationException(String.format("Service Function Chain '%s'",


This msg does not make sense.... Service Function Chain ?? The message must be a complete statement.

Change1:Reframed the Error Message. Change2:Renamed the test methods. Change3:Added the TODO comments for future references. Change4:Reframed the throw message

bmmukund · 2018-01-12T23:07:45Z

@emanoelxavier ,
I did the required changes.Related to java doc,As Arvind asked me to include.Hence,i am not removing Please review the code and let me know if any changes needs to be done.

sudhirappaji · 2018-01-14T06:13:21Z

@bmmukund ,please update the branch, I will take a look and merge it on Monday

bmmukund requested review from emanoelxavier, arvindn05 and sudhirappaji December 12, 2017 10:28

sudhirappaji reviewed Dec 12, 2017

View reviewed changes

emanoelxavier suggested changes Dec 13, 2017

View reviewed changes

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch 2 times, most recently from 05a5179 to 493c328 Compare December 18, 2017 12:00

emanoelxavier suggested changes Dec 18, 2017

View reviewed changes

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch 2 times, most recently from acccb63 to cc54476 Compare December 21, 2017 08:40

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch from cc54476 to 239c8b6 Compare December 27, 2017 09:15

arvindn05 previously approved these changes Dec 27, 2017

View reviewed changes

emanoelxavier suggested changes Dec 28, 2017

View reviewed changes

bmmukund dismissed arvindn05’s stale review via 2d8bd85 December 28, 2017 18:31

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch 4 times, most recently from 0abf5f1 to 63b7764 Compare December 28, 2017 20:41

Fixed the issue osc-core issue opensecuritycontroller#543

06eb96e

Modified and added test cases. Added back to original intialization Made the changes according to the review comments. Did required changes for unit test case. Did the changes as per review comments.

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch from 63b7764 to 06eb96e Compare December 29, 2017 07:29

arvindn05 requested changes Dec 29, 2017

View reviewed changes

emanoelxavier suggested changes Jan 3, 2018

View reviewed changes

Changes are incorporated as per the review comments.

d514a8d

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch from 4a770f4 to d514a8d Compare January 12, 2018 11:17

bmmukund self-assigned this Jan 12, 2018

bmmukund closed this Jan 12, 2018

sudhirappaji reopened this Jan 12, 2018

emanoelxavier suggested changes Jan 12, 2018

View reviewed changes

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch 3 times, most recently from 22cacec to 14d1494 Compare January 12, 2018 22:12

emanoelxavier suggested changes Jan 12, 2018

View reviewed changes

Changes have done as per review comments.

25162a9

Change1:Reframed the Error Message. Change2:Renamed the test methods. Change3:Added the TODO comments for future references. Change4:Reframed the throw message

bmmukund force-pushed the osc_core_DA_SFC_Error_Fix branch from abfab86 to 25162a9 Compare January 12, 2018 23:02

emanoelxavier approved these changes Jan 12, 2018

View reviewed changes

Merge branch 'master' into osc_core_DA_SFC_Error_Fix

1512249

sudhirappaji approved these changes Jan 15, 2018

View reviewed changes

sudhirappaji merged commit 2e8ac34 into opensecuritycontroller:master Jan 15, 2018

		@@ -185,7 +185,7 @@ public static synchronized Long generateUniqueTag(EntityManager em, VirtualSyste
		}

		public static boolean isProtectingWorkload(VirtualSystem vs) {


		}

		private void validateSfcReferenceToSecurityGroup(EntityManager em, DistributedAppliance da,


		@RunWith(PowerMockRunner.class)

Fixed the issue osc-core issue #543 #621

Fixed the issue osc-core issue #543 #621

Conversation

bmmukund commented Dec 12, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

bmmukund commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudhirappaji commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmmukund commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindn05 commented Dec 21, 2017 • edited Loading

codecov bot commented Dec 27, 2017 • edited Loading

Codecov Report

bmmukund commented Dec 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanoelxavier Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmmukund commented Dec 29, 2017

sudhirappaji commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmmukund commented Jan 12, 2018

sudhirappaji commented Jan 12, 2018

Choose a reason for hiding this comment

emanoelxavier Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmmukund commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmmukund commented Jan 12, 2018

sudhirappaji commented Jan 14, 2018

bmmukund commented Dec 12, 2017 •

edited

Loading

arvindn05 commented Dec 21, 2017 •

edited

Loading

codecov bot commented Dec 27, 2017 •

edited

Loading

emanoelxavier Dec 28, 2017 •

edited

Loading

emanoelxavier Jan 12, 2018 •

edited

Loading