Update patch set 15
Patch Set 15: Code-Review-1 (11 comments) I know it's WIP, but I hope this review will be helpful. I like the idea, but I have some concerns of airshipctl doing _too much_. Please see my inline comments. Patch-set: 15 Reviewer: Gerrit User 17068 <17068@4a232e18-c5a9-48ee-94c0-e04e7cca6543> Label: Code-Review=-1
This commit is contained in:
Gerrit User 17068
Gerrit Code Review
parent
654b7915f5
commit
b6f1a5ff71
|
|
@ -0,0 +1,233 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_962d06aa",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 36,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "Very good definition of the problem description, and of the goals.",
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_d65e1e44",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 73,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "Not sure if these last parts should be the _duty_ of airshipctl.\n\nIMO, we should let the deployers decide how things will be pushed. Maybe the deployer already have a way to push manifests into an environment.\nWe are not so far from a `kubectl apply` away, so I am not sure if there is real value by implementing the same features into Airshipctl.\n\nThe airshipctl can be considered as a smart client into the handling of manifests, which can then give the duty to other tools.\n\nFor the workflow case, airshipctl can generate the workflow documents, the deployer then deploys the resulting CRDs document into its environment.",
|
||||
"range": {
|
||||
"startLine": 72,
|
||||
"startChar": 40,
|
||||
"endLine": 73,
|
||||
"endChar": 23
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_512798b7",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 78,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "That will open the door to many things, including forks of some part of the code.\n\nThis helps adoption (everyone can do its own) but increases the risk of not upstreaming code by some contributors.\n\nI believe, in this case, that it\u0027s better to have a plugin system, in order to increase adoption.\n\nLong story short: I like it.",
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_112120a0",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 84,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "what does this mean?",
|
||||
"range": {
|
||||
"startLine": 84,
|
||||
"startChar": 11,
|
||||
"endLine": 84,
|
||||
"endChar": 26
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_51755897",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 92,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "Could you clarify this?\n\nAssuming airshipctl manipulates k8s manifests (and not k8s objects), there is no need to implement all of this. Assuming it manipulates k8s objects in a live cluster, isn\u0027t the k8s context be enough?",
|
||||
"range": {
|
||||
"startLine": 91,
|
||||
"startChar": 0,
|
||||
"endLine": 92,
|
||||
"endChar": 66
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_519a38c5",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 99,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "that is very convenient for users. I like it. Not sure this should be exposed as part of the base cli or part of a library, and re-used in different cli plugins.",
|
||||
"range": {
|
||||
"startLine": 98,
|
||||
"startChar": 51,
|
||||
"endLine": 99,
|
||||
"endChar": 21
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_71865468",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 119,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "Will it be possible to install airshipctl without having all the subcommands? How can this be packaged?",
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_d1adc8ea",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 121,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "shouldn\u0027t this be another spec file, as the implementation of an official config plugin?",
|
||||
"range": {
|
||||
"startLine": 121,
|
||||
"startChar": 0,
|
||||
"endLine": 121,
|
||||
"endChar": 6
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_31a77c0e",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 137,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "can we consider this config a global sites manifest? Or do you want to strip this down into a config and manifests? If yes, could you clarify why?",
|
||||
"range": {
|
||||
"startLine": 137,
|
||||
"startChar": 38,
|
||||
"endLine": 137,
|
||||
"endChar": 124
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_91b7503b",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 357,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "nit: whitespaces",
|
||||
"range": {
|
||||
"startLine": 357,
|
||||
"startChar": 0,
|
||||
"endLine": 357,
|
||||
"endChar": 3
|
||||
},
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
},
|
||||
{
|
||||
"key": {
|
||||
"uuid": "7faddb67_51e8f814",
|
||||
"filename": "specs/approved/airshipctl.rst",
|
||||
"patchSetId": 15
|
||||
},
|
||||
"lineNbr": 542,
|
||||
"author": {
|
||||
"id": 17068
|
||||
},
|
||||
"writtenOn": "2019-07-26T11:47:38Z",
|
||||
"side": 1,
|
||||
"message": "See also comment above.",
|
||||
"revId": "2a58dd3457df06aa11dc1e63a0aa7acd768068b2",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
|
||||
"unresolved": false
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in New Issue