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

[RM] Fix Issue#647 and Issue#654 #657

Closed

Conversation

kevin-zhonghao
Copy link
Contributor

Fix Issue #647 :

  1. Rename method
  2. Separate the part of create default subnet route table if it can't find it in database.

and Issue #654 :

  1. Rename API and related method.
  2. Consolidate POST with create neutron/vpc subnet route table
  3. Add validation of input in CreateSubnetRouteTable

@xieus xieus added the bug Something isn't working label Jun 30, 2021
@xieus xieus added this to the Version 0.17.2021.07.30 milestone Jun 30, 2021
@cj-chung cj-chung requested review from cj-chung and xieus June 30, 2021 16:34
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.

@kevin-zhonghao It seems like @DavidLiu506's PR#655 has most of change included in your PR. Please just review @DavidLiu506's PR, put your change in his PR, and discard your current PR.

Thanks.

@@ -315,7 +315,7 @@ public RouteTableWebJson getVpcRouteTableById(@PathVariable String projectid, @P
method = GET,
value = {"/project/{projectid}/subnets/{subnetid}/routetable"})
@DurationStatistics
public RouteTableWebJson getOrCreateSubnetRouteTable(@PathVariable String projectid, @PathVariable String subnetid) throws Exception {
public RouteTableWebJson getSubnetRouteTable(@PathVariable String projectid, @PathVariable String subnetid) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kevin-zhonghao I think @DavidLiu506 also modify this function, please check his PR#655

@@ -350,7 +350,7 @@ public RouteTableWebJson getOrCreateSubnetRouteTable(@PathVariable String projec
method = POST,
value = {"/project/{projectid}/subnets/{subnetid}/routetable"})
@DurationStatistics
public RouteTableWebJson createNeutronSubnetRouteTable(@PathVariable String projectid, @PathVariable String subnetid, @RequestBody RouteTableWebJson resource) throws Exception {
public RouteTableWebJson createSubnetRouteTable(@PathVariable String projectid, @PathVariable String subnetid, @RequestBody RouteTableWebJson resource) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kevin-zhonghao Please check PR#655

@@ -378,7 +378,7 @@ public RouteTableWebJson createNeutronSubnetRouteTable(@PathVariable String proj
"0.0.0.0/0", null, 100, null, "192.168.0.1");
routes.add(defaultRoute);

routeTable = this.routerService.createNeutronSubnetRouteTable(projectid, subnetid, resource, routes);
routeTable = this.routerService.createSubnetRouteTable(projectid, subnetid, resource, routes);
Copy link
Contributor

Choose a reason for hiding this comment

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

@kevin-zhonghao Please remove the default route in line 377.

routeTable = this.routeTableDatabaseService.getByRouteTableId(vpcDefaultRouteTableId);
return routeTable;
} else if (routeTableMap.size() > 1) {
if (routeTableMap.size() > 1) {
Copy link
Contributor

@cj-chung cj-chung Jun 30, 2021

Choose a reason for hiding this comment

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

@kevin-zhonghao I think @DavidLiu506 has pretty much the same change in his PR. If most of the changes are already included in @DavidLiu506's PR, please just review @DavidLiu506's PR and discard your current PR.

@cj-chung
Copy link
Contributor

cj-chung commented Jul 7, 2021

This PR is already included in PR #655, we can close this PR.

@kevin-zhonghao
Copy link
Contributor Author

This PR is already included in PR #655, we can close this PR.

No problem, I'll quickly check the PR #655 and then close it

@kevin-zhonghao
Copy link
Contributor Author

Have reviewed PR #655 and #655 has already covered all change in this PR.
So close the PR #657

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants