openstack
/
neutron-lbaas-dashboard
Code Issues Proposed changes

Update patch set 7 

Patch Set 7: Code-Review-1

(18 comments)

Most of my comments are minor. I haven't tried this with my devstack env. I will try it later.

Patch-set: 7
Label: Code-Review=-1
This commit is contained in:
Gerrit User 841 2015-09-22 19:57:41 +00:00 committed by Gerrit Code Review
parent 9f2ca26ad7
commit 76b0e71172
1 changed files with 334 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

334
2e5d3e77f7bf7209ba9655631236060fec81fca8 Normal file
View File

@ -0,0 +1,334 @@
{
"comments": [
{
"key": {
"uuid": "ba15a1d1_83821251",
"filename": "neutron_lbaas_dashboard/api/lbaasv2.py",
"patchSetId": 7
},
"lineNbr": 55,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "This method is specific to LBaaS v1 dashboard and I cannot remember why this method is necessary for LBaaS v1 dashboard. Perhaps we can remove them all, but it can be deferred. All other API layers in horizon do not have such method.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_dd530938",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/panel.py",
"patchSetId": 7
},
"lineNbr": 28,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "I prefer to TODO rather than todo. Consistent usage of upper case helps us to find more TODOs :-)\n\nNote that I am fine with always enabling LBaaS v2 dashboard because LBaaS v2 dashboard is enabled only when operators enable it manually in the configuration.",
"range": {
"startLine": 28,
"startChar": 10,
"endLine": 28,
"endChar": 14
},
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_680c7d3b",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 16,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "No blank line is necessary here.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_48a5f901",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 34,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "Why don\u0027t you use \"Delete\" ? I think it is consistent and users can know a given load balancer will be deleted more easily. As non-native speaker, I am not sure \"terminate\" associates user with \"delete\" operation.",
"range": {
"startLine": 34,
"startChar": 23,
"endLine": 34,
"endChar": 35
},
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_4835f943",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 38,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "action_past and related attributes for BatchAction are now deprecated.\nAn example of the new usage is found at:\nhttps://github.com/openstack/horizon/blob/master/openstack_dashboard/dashboards/project/networks/tables.py#L44-L60",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_48d179cc",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 62,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "Where is api.lbui defined? api.lbaasv2 instead?",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_e8e08d98",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 77,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "We can just do:\n\n return loadbalancer.admin_state_up",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_c8810991",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 120,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "Same as the below.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_087861c3",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/tables.py",
"patchSetId": 7
},
"lineNbr": 124,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "I think it is better we have a docstring which describe what this method does with examples.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_e2718c45",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/templates/loadbalancersv2/_detail_overview.html",
"patchSetId": 7
},
"lineNbr": 34,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "L.31-34: We have the same contents at L.19-22. Do we still need it here?",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_02cb6054",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/templates/loadbalancersv2/_detail_overview.html",
"patchSetId": 7
},
"lineNbr": 48,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "When we have no member, it is nice if we see \u0027None\u0027 or \u0027-\u0027.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_02d28000",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/templates/loadbalancersv2/_monitor_create.html",
"patchSetId": 7
},
"lineNbr": 17,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "Do we still need a comment?",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_5db7f9f4",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 46,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "When you do not pass \"redirect\" in exceptions.handle(), the processing continues, so you need \"pools \u003d []\" here.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_9d2ef1f8",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 72,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "Instead of using self._object, I think you can use memoized decorator.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_3dab258b",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 74,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "For debug message, use LOG.debug or remove it.",
"range": {
"startLine": 74,
"startChar": 11,
"endLine": 74,
"endChar": 20
},
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_7d12fdb5",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 78,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "I wonder why we need to use readable() method.\nIt is a convention which is used only in LBaaS v1 dashboard.\nWe can fix it later, but once we fix it get_initial below would be much simpler.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_dd71c945",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 82,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "e.message is the style which was used in python 2.6 and it is now deprecated. Use just \"e\" instead. %s handles it properly.",
"range": {
"startLine": 82,
"startChar": 22,
"endLine": 82,
"endChar": 31
},
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "ba15a1d1_9d15919c",
"filename": "neutron_lbaas_dashboard/dashboards/project/loadbalancersv2/views.py",
"patchSetId": 7
},
"lineNbr": 128,
"author": {
"id": 841
},
"writtenOn": "2015-09-22T19:57:41Z",
"side": 1,
"message": "As commented above, we can use @memoized decorator instead.",
"revId": "2e5d3e77f7bf7209ba9655631236060fec81fca8",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
}
]
}