x
/
fuel-plugin-calico
Code Issues Proposed changes

Update patch set 5 

Patch Set 5: Code-Review-1

(19 comments)

Wow, amazing work here; thanks so much for working on this!

I don't understand 100%, but basically this all looks reasonable to me.  I have left quite a few questions below - for the sake of improving my own understanding - and a few nits/typos etc.  I would appreciate if you could answer those questions before we merge this, hence the -1.

Also I'd really like to try this out.  Can you advise what is the simplest way to do that?  Is it possible to try Fuel on top of GCE or AWS resources?  (Unfortunately I don't currently have access to a VMware setup.)

Patch-set: 5
Label: Code-Review=-1
This commit is contained in:
Gerrit User 13734 2016-09-14 16:47:07 +00:00 committed by Gerrit Code Review
parent e9041de0b4
commit 49cddbf19a
1 changed files with 327 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

327
2162f4a24a506d529ec575e106c3e8652d9ef832 Normal file
View File

@ -0,0 +1,327 @@
{
"comments": [
{
"key": {
"uuid": "9a89bdaa_41994e78",
"filename": "/COMMIT_MSG",
"patchSetId": 5
},
"lineNbr": 9,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "nit: s/an/a/",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "9a89bdaa_e128a2e4",
"filename": "README.md",
"patchSetId": 5
},
"lineNbr": 87,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "s/for/to/",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "9a89bdaa_7c799592",
"filename": "components.yaml",
"patchSetId": 5
},
"lineNbr": 6,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What does \u0027tun\u0027 mean?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_2a6f3fc6",
"filename": "deployment_scripts/puppet/manifests/compute_neutron_nova.pp",
"patchSetId": 5
},
"lineNbr": 9,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "How much calico-specific stuff is there in this file? It seems a shame to duplicate so much of the standard Nova setup.",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_5f736f80",
"filename": "deployment_scripts/puppet/manifests/neutron_server_config.pp",
"patchSetId": 5
},
"lineNbr": 2,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "Is it really necessary to duplicate most of neutron.conf here? In most deployment approaches we only need to change ml2_conf.ini.",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_7f0f4b2e",
"filename": "deployment_scripts/puppet/manifests/private_gateway_check.pp",
"patchSetId": 5
},
"lineNbr": 1,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What is happening here?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_dceead8a",
"filename": "deployment_scripts/puppet/modules/calico/Modulefile",
"patchSetId": 5
},
"lineNbr": 2,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What does this version pertain to?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_2dc5a43b",
"filename": "deployment_scripts/puppet/modules/calico/manifests/params.pp",
"patchSetId": 5
},
"lineNbr": 1,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "2016?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_0df6288f",
"filename": "deployment_scripts/puppet/modules/calico/manifests/params.pp",
"patchSetId": 5
},
"lineNbr": 18,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "In other places you\u0027ve changed \u0027calico-fuel-plugin\u0027 to \u0027fuel-plugin-calico\u0027. Should that be done here too?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_50b7e738",
"filename": "deployment_scripts/puppet/modules/calico/manifests/params.pp",
"patchSetId": 5
},
"lineNbr": 27,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "This variable appears to be unused. The correct location is the calico-1.4 PPA, as you have in repo_setup.pp",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_5c97fd8f",
"filename": "deployment_scripts/puppet/modules/calico/manifests/params.pp",
"patchSetId": 5
},
"lineNbr": 37,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What is this IP used for?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_1c42d510",
"filename": "deployment_scripts/puppet/modules/calico/manifests/params.pp",
"patchSetId": 5
},
"lineNbr": 44,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What is this mgmt IP/interface used for, and how is it different from the one above?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_2da0449f",
"filename": "deployment_scripts/puppet/modules/calico/templates/bird-compute.conf.erb",
"patchSetId": 5
},
"lineNbr": 24,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "What is the br-mesh device?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_0d1e280c",
"filename": "deployment_scripts/puppet/modules/calico/templates/bird-peer-rr.conf.erb",
"patchSetId": 5
},
"lineNbr": 10,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "FWIW I think source address can usually be omitted. BIRD will use the source address that is appropriate for reaching remote_ipaddr.",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_cd7360b0",
"filename": "deployment_scripts/puppet/modules/calico/templates/calico-alt-gateway.conf.erb",
"patchSetId": 5
},
"lineNbr": 2,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "I\u0027m afraid I don\u0027t understand this. Could you explain more about what this means and why it is needed?",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_17f6398f",
"filename": "environment_config.yaml",
"patchSetId": 5
},
"lineNbr": 4,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "nits: s/Openstack/OpenStack/\ns/instead/instead of/",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_9a2b5082",
"filename": "environment_config.yaml",
"patchSetId": 5
},
"lineNbr": 43,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "Why does this condition have !\u003d ? I would guess (by analogy with the other condition for enable_ipv6) that it should be \u003d\u003d",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_b7cae59f",
"filename": "node_roles.yaml",
"patchSetId": 5
},
"lineNbr": 18,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "Etcd clusters should always be odd numbers, so 2 doesn\u0027t make sense here; I suggest either 1 or 3.",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "7a8ec9b2_45abaa1d",
"filename": "specs/calico-fuel-plugin.rst",
"patchSetId": 5
},
"lineNbr": 148,
"author": {
"id": 13734
},
"writtenOn": "2016-09-14T16:47:07Z",
"side": 1,
"message": "This mailing list is no longer in use, so please delete this bullet.",
"revId": "2162f4a24a506d529ec575e106c3e8652d9ef832",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
}
]
}