python-tripleoclient/229f89f4354d04f6d484b42673a4fd1e0f355427

{
  "comments": [
    {
      "unresolved": true,
      "key": {
        "uuid": "f80fb19a_c0cb2f07",
        "filename": "/COMMIT_MSG",
        "patchSetId": 5
      },
      "lineNbr": 19,
      "author": {
        "id": 30073
      },
      "writtenOn": "2023-03-06T23:07:13Z",
      "side": 1,
      "message": "Resolves: rhbz#2166224",
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": false,
      "key": {
        "uuid": "4e8dec27_d819e1c1",
        "filename": "/PATCHSET_LEVEL",
        "patchSetId": 5
      },
      "lineNbr": 0,
      "author": {
        "id": 30073
      },
      "writtenOn": "2023-03-06T23:07:13Z",
      "side": 1,
      "message": "Couple of things I think we can improve here before merging.\n\nI would like to get some additional feedback from others on a more centralised handling of the `excluded_overcloud` group as per my comment on the overcloud_update.py change at #204.",
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "5ba97af7_76c87c4a",
        "filename": "tripleoclient/v1/overcloud_update.py",
        "patchSetId": 5
      },
      "lineNbr": 102,
      "author": {
        "id": 30073
      },
      "writtenOn": "2023-03-06T23:07:13Z",
      "side": 1,
      "message": "What is skiplist in this context? It makes sense when we refer to it here:\nhttps://github.com/openstack/python-tripleoclient/blob/master/tripleoclient/v1/overcloud_node.py#L133-L141\n\nBecause we\u0027re defining what skip list is. But I feel like it\u0027s not quite clear which hosts will be excluded based on this description. Maybe it would be better to say something like, \"If nodes in the --limit group have already been excluded via the DeploymentServerBlacklist parameter, they will be skipped during the execution\"",
      "range": {
        "startLine": 100,
        "startChar": 0,
        "endLine": 102,
        "endChar": 37
      },
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "059f9dd3_f34d37e7",
        "filename": "tripleoclient/v1/overcloud_update.py",
        "patchSetId": 5
      },
      "lineNbr": 102,
      "author": {
        "id": 9816
      },
      "writtenOn": "2023-03-07T06:02:50Z",
      "side": 1,
      "message": "+1. skiplist is not an external interface so we should refer to DeploymentServerBlacklist instead.",
      "parentUuid": "5ba97af7_76c87c4a",
      "range": {
        "startLine": 100,
        "startChar": 0,
        "endLine": 102,
        "endChar": 37
      },
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "5d8e3a95_a61cccef",
        "filename": "tripleoclient/v1/overcloud_update.py",
        "patchSetId": 5
      },
      "lineNbr": 204,
      "author": {
        "id": 30073
      },
      "writtenOn": "2023-03-06T23:07:13Z",
      "side": 1,
      "message": "I wonder if we should just do this by default in the `run_ansible_playbook` method? Is there ever a situation where we want to execute against the `excluded_overcloud` group?\n\nFeels like that would simplify the handling of this across the project by avoiding the need for each module to implement handling independently.",
      "range": {
        "startLine": 199,
        "startChar": 0,
        "endLine": 204,
        "endChar": 62
      },
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "039070e0_f3c63bee",
        "filename": "tripleoclient/v1/overcloud_update.py",
        "patchSetId": 5
      },
      "lineNbr": 204,
      "author": {
        "id": 9816
      },
      "writtenOn": "2023-03-07T06:02:50Z",
      "side": 1,
      "message": "Although we can add the expression to exclude the excluded_overcloud always, I tend to agree with the warning which also contains list of node names. In case a node is blacklisted then the node is not updated. Users should be aware of that fact, otherwise they might leave some node with older version.\n\nAlso, does it makes sense to make the logic fail if user gives a specific node which is blacklisted ? That would also make the use aware of the fact the request can\u0027t be fulfilled.",
      "parentUuid": "5d8e3a95_a61cccef",
      "range": {
        "startLine": 199,
        "startChar": 0,
        "endLine": 204,
        "endChar": 62
      },
      "revId": "229f89f4354d04f6d484b42673a4fd1e0f355427",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    }
  ]
}