openstack
/
neutron-vpnaas
Code Issues Proposed changes
neutron-vpnaas/f9bc11d0ba4bbdaadcd6097eceb...

227 lines
8.8 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "2a482897_88f8611d",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 32755
},
"writtenOn": "2023-09-12T07:04:29Z",
"side": 1,
"message": "Hello Bodo, sorry for dragging you in as a reviewer this bluntly. I unfortunately did not pursue this patch for a while. But since you did the OVN patch for VPNaaS (https://review.opendev.org/c/openstack/neutron-vpnaas/+/765353), and that is \"almost\" merged, I\u0027d kindly like to ask you to take a peek at my ideas an changes here and maybe give some feedback for things I should change.\n\nIf the approach looks good and valuable to you, I\u0027d really like to finish this (tests failing) to make VPNaaS services more stable.",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "f3c66170_296db31c",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 31976
},
"writtenOn": "2023-09-13T11:21:28Z",
"side": 1,
"message": "I started to look at it. Maybe you can change the `sync` method back to take a router dict.",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "b6dc1aad_7e47e4a8",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 4
},
"lineNbr": 0,
"author": {
"id": 32755
},
"writtenOn": "2023-09-26T07:36:29Z",
"side": 1,
"message": "Thanks a bunch for your time and feedback Bodo!\nSorry for my late response. I shall take another slash at this one soon.",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "06310f9f_1f3d0b6b",
"filename": "neutron_vpnaas/services/vpn/agent.py",
"patchSetId": 4
},
"lineNbr": 77,
"author": {
"id": 31976
},
"writtenOn": "2023-09-13T11:21:28Z",
"side": 1,
"message": "Instead of passing the router info (ri), keep the old way with a router dict that at least contains the \u0027id\u0027. There are other code paths that call sync still in the old way. And some tests fail because of that.\nTo update the device_driver\u0027s router cache with the updated info, I\u0027d suggest to add a device_driver method `update_router(ri)`.\nI\u0027m also thinking about how a change would impact my vpnaas-for-ovn patch and there I don\u0027t really need the router info",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "e8249abe_a7938aae",
"filename": "neutron_vpnaas/services/vpn/agent.py",
"patchSetId": 4
},
"lineNbr": 77,
"author": {
"id": 32755
},
"writtenOn": "2024-02-12T17:02:43Z",
"side": 1,
"message": "The add_router method was, as far as I can see always storing the router_info, while the sync method was not involved in updating the self.routers dict.\n\nWith this change here, I added updating the stored / cached routers (which are actually router info objects if I am not mistaken).\n\nIn the end I simply want to align the method signatures a little more and work on the type types of objects everywhere. \n\nIf you have the time, please check out my recent patchset. I truly intented to increase my responsiveness, in comments and code.",
"parentUuid": "06310f9f_1f3d0b6b",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "5db8de64_9082edff",
"filename": "neutron_vpnaas/services/vpn/device_drivers/strongswan_ipsec.py",
"patchSetId": 4
},
"lineNbr": 196,
"author": {
"id": 31976
},
"writtenOn": "2023-09-13T15:01:10Z",
"side": 1,
"message": "`self._execute` only returns stdout, so I assume you will get a `ValueError: too many values to unpack (expected 2)` here. Internally `execute` in neutron.agent.linux.utils is called without setting `return_stderr\u003dTrue`.",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "27d158d5_91389a06",
"filename": "neutron_vpnaas/services/vpn/device_drivers/strongswan_ipsec.py",
"patchSetId": 4
},
"lineNbr": 196,
"author": {
"id": 32755
},
"writtenOn": "2024-02-12T17:02:43Z",
"side": 1,
"message": "Done",
"parentUuid": "5db8de64_9082edff",
"revId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
],
"submitRequirementResults": [
{
"submitRequirement": {
"name": "Code-Review",
"description": {
"value": "Code reviewed by core reviewer"
},
"applicabilityExpression": {},
"submittabilityExpression": {
"expressionString": "label:Code-Review\u003dMAX AND -label:Code-Review\u003dMIN"
},
"overrideExpression": {},
"allowOverrideInChildProjects": true
},
"applicabilityExpressionResult": {},
"submittabilityExpressionResult": {
"value": {"expression":{"expressionString":"label:Code-Review=MAX AND -label:Code-Review=MIN"},"status":"FAIL","errorMessage":{"value":null},"passingAtoms":[],"failingAtoms":["label:Code-Review=MAX","label:Code-Review=MIN"]}
},
"overrideExpressionResult": {},
"patchSetCommitId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"legacy": {
"value": false
},
"forced": {},
"hidden": {}
},
{
"submitRequirement": {
"name": "Review-Priority",
"description": {
"value": "Review priority"
},
"applicabilityExpression": {},
"submittabilityExpression": {
"expressionString": "-label:Review-Priority\u003dMIN"
},
"overrideExpression": {},
"allowOverrideInChildProjects": false
},
"applicabilityExpressionResult": {},
"submittabilityExpressionResult": {
"value": {"expression":{"expressionString":"-label:Review-Priority=MIN"},"status":"PASS","errorMessage":{"value":null},"passingAtoms":[],"failingAtoms":["label:Review-Priority=MIN"]}
},
"overrideExpressionResult": {},
"patchSetCommitId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"legacy": {
"value": false
},
"forced": {},
"hidden": {}
},
{
"submitRequirement": {
"name": "Verified",
"description": {
"value": "Verified in gate by CI"
},
"applicabilityExpression": {},
"submittabilityExpression": {
"expressionString": "label:Verified\u003dMAX AND -label:Verified\u003dMIN"
},
"overrideExpression": {},
"allowOverrideInChildProjects": false
},
"applicabilityExpressionResult": {},
"submittabilityExpressionResult": {
"value": {"expression":{"expressionString":"label:Verified=MAX AND -label:Verified=MIN"},"status":"FAIL","errorMessage":{"value":null},"passingAtoms":[],"failingAtoms":["label:Verified=MAX","label:Verified=MIN"]}
},
"overrideExpressionResult": {},
"patchSetCommitId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"legacy": {
"value": false
},
"forced": {},
"hidden": {}
},
{
"submitRequirement": {
"name": "Workflow",
"description": {
"value": "Approved for gate by core reviewer"
},
"applicabilityExpression": {},
"submittabilityExpression": {
"expressionString": "label:Workflow\u003dMAX AND -label:Workflow\u003dMIN"
},
"overrideExpression": {},
"allowOverrideInChildProjects": false
},
"applicabilityExpressionResult": {},
"submittabilityExpressionResult": {
"value": {"expression":{"expressionString":"label:Workflow=MAX AND -label:Workflow=MIN"},"status":"FAIL","errorMessage":{"value":null},"passingAtoms":[],"failingAtoms":["label:Workflow=MAX","label:Workflow=MIN"]}
},
"overrideExpressionResult": {},
"patchSetCommitId": "f9bc11d0ba4bbdaadcd6097eceb32e3276602bb9",
"legacy": {
"value": false
},
"forced": {},
"hidden": {}
}
]
}
View Git Blame Copy Permalink