Skip to content
This repository has been archived by the owner on Mar 31, 2023. It is now read-only.

[Port Mgr] Support multiple ResourceOperation types #603

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

xieus
Copy link
Contributor

@xieus xieus commented Apr 29, 2021

In PR #602, a new ResourceOperationType field was added in NetworkConfiguration. This PR sets the value of ResourceOperationTypes when building network configuration and passes down to DPM.

@xieus xieus added enhancement New feature or request feature feature development labels Apr 29, 2021
@xieus xieus added this to the Version 0.12.2021.04.30 milestone Apr 29, 2021
@xieus xieus requested a review from cj-chung April 29, 2021 04:43
@xieus xieus self-assigned this Apr 29, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #603 (4b9573e) into master (a9e3d8b) will increase coverage by 0.09%.
The diff coverage is 78.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #603      +/-   ##
============================================
+ Coverage     32.65%   32.75%   +0.09%     
- Complexity     1241     1248       +7     
============================================
  Files           522      522              
  Lines         13093    13117      +24     
  Branches       1606     1610       +4     
============================================
+ Hits           4276     4296      +20     
- Misses         8235     8236       +1     
- Partials        582      585       +3     
Impacted Files Coverage Δ Complexity Δ
...rewei/alcor/portmanager/processor/PortContext.java 67.53% <33.33%> (-1.39%) 32.00 <1.00> (+1.00) ⬇️
...lcor/portmanager/processor/DataPlaneProcessor.java 69.72% <84.00%> (+1.43%) 39.00 <7.00> (+5.00)
...alcor/portmanager/util/RestParameterValidator.java 46.15% <0.00%> (+3.07%) 14.00% <0.00%> (+1.00%)

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 a9e3d8b...4b9573e. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Apr 29, 2021

This pull request introduces 1 alert when merging 4b9573e into a9e3d8b - view on LGTM.com

new alerts:

  • 1 for Spurious Javadoc @param tags

Copy link
Contributor

@cj-chung cj-chung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xieus xieus changed the title [Port Mgr] Utilize latest NetworkConfiguration contract and set ResourceOperationTypes [Port Mgr] Support multiple ResourceOperation types Apr 29, 2021
@xieus xieus merged commit 5f71b54 into futurewei-cloud:master Apr 29, 2021
@xieus xieus deleted the pm/resourcetype branch April 29, 2021 16:41
}

if (context.containRouters()) {
resourceOperationTypes.add(new ResourceOperation(Common.ResourceType.NEIGHBOR, Common.OperationType.CREATE));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resrource type NEIHBOR was already added at line 236, this looks like a duplicate. From the context it looks like resource type ROUTER might be the correct one.
If this is in fact, the correct resource type, a clarifying comment will remove the confusion.

@@ -278,14 +313,15 @@ private void updateNetworkConfig(PortContext context, NetworkConfiguration netwo
networkConfig.setOpType(Common.OperationType.UPDATE);
IRestRequest updateNetworkConfigRequest =
new UpdateNetworkConfigRequest(context, networkConfig);
context.getRequestManager().sendRequestAsync(updateNetworkConfigRequest, request -> portService.updatePortStatus(request,networkConfig,null));
portService.updatePortStatus(updateNetworkConfigRequest,networkConfig, StatusEnum.PENDING.getStatus());
context.getRequestManager().sendRequestAsync(updateNetworkConfigRequest, request -> portService.updatePortStatus(request, networkConfig, null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, update doesn't require multiple resource types, then. A comment explicitley stating this fact would be helpful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request feature feature development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants