Update patch set 11

Patch Set 11: Code-Review-1

(6 comments)

Some suggestions in addition to inline comments:

- Break up this patch into incremental changes. Removing the unneeded files can be a change on its own, for example. Explain each change in their commit messages.
- Omit all the vagrant stuff. It's not needed for infra to run it and none of the other puppet modules have it.

Patch-set: 11
Reviewer: Gerrit User 8482 <8482@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1
This commit is contained in:
Gerrit User 8482 2016-08-02 21:11:57 +00:00 committed by Gerrit Code Review
parent 53f35f04e5
commit f55e4cde88
1 changed files with 102 additions and 0 deletions

View File

@ -1,5 +1,22 @@
{
"comments": [
{
"key": {
"uuid": "bacf61ea_663b9539",
"filename": "/COMMIT_MSG",
"patchSetId": 11
},
"lineNbr": 7,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "This seems to be just one patch",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_3140e406",
@ -23,6 +40,40 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_e63ae597",
"filename": "manifests/init.pp",
"patchSetId": 11
},
"lineNbr": 45,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "Is this really the right email?",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_66c0b57d",
"filename": "manifests/init.pp",
"patchSetId": 11
},
"lineNbr": 49,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "This is the wrong way to share variables across classes. Omit the vars class and instead pass each variable directly to the classes that use them, which you\u0027re declaring below anyway.",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_51491830",
@ -69,6 +120,57 @@
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_c6ac018f",
"filename": "manifests/install.pp",
"patchSetId": 11
},
"lineNbr": 126,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "This needs a creates, onlyif, unless, or refreshonly parameter so that it doesn\u0027t run on every puppet run.",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_068e1927",
"filename": "manifests/install.pp",
"patchSetId": 11
},
"lineNbr": 134,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "This needs a creates, onlyif, unless, or refreshonly parameter so that it doesn\u0027t run on every puppet run.",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_06873913",
"filename": "manifests/install.pp",
"patchSetId": 11
},
"lineNbr": 144,
"author": {
"id": 8482
},
"writtenOn": "2016-08-02T21:11:57Z",
"side": 1,
"message": "It would be better not to manage a file inside a vcsrepo. At best there\u0027s an untracked file in the git repo and at worst the source repo might end up with a file of the same name and will end up causing merge conflicts when the repo needs to be pulled.",
"revId": "9489bc2662bc4e8ab6094b6ea94d25e34c3d2328",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
"unresolved": false
},
{
"key": {
"uuid": "bacf61ea_511e380c",