charm-vault/b2d1dddc27f8f71382a2ff68f56470b51dcdf986

{
  "comments": [
    {
      "unresolved": true,
      "key": {
        "uuid": "49d0bfa2_e1b8c956",
        "filename": "/COMMIT_MSG",
        "patchSetId": 2
      },
      "lineNbr": 8,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T12:58:06Z",
      "side": 1,
      "message": "This commit message needs expanding, please, with a description of *why* the feature is being added, *how* it impacts usage, and *what* effects it produces.  e.g. it overrides the \"crl_distribution_points\" parameter in the \"client.secrets.pki.set_urls(...)\" call to vault.\n\nIs there a related bug to this feature.  If so, and this patch fully resolves the feature/bug, then please add:\n\nCloses-bug: #\u003cLP bug number\u003e\n\nThe purpose of the commit message is to allow a reader of the patch to understand why the patch is being added, what impact it has, and any implications for existing installations.",
      "range": {
        "startLine": 6,
        "startChar": 0,
        "endLine": 8,
        "endChar": 0
      },
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "16e3aed9_d46b80ff",
        "filename": "/COMMIT_MSG",
        "patchSetId": 2
      },
      "lineNbr": 8,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-08T13:24:00Z",
      "side": 1,
      "message": "This commit message still needs work.  Just pointing to a bug creates work for other people when they are viewing the commit message.\n\nPlease see the following for examples:\n\n- https://docs.opendev.org/opendev/infra-manual/latest/developers.html#committing-changes\n- Post on how to write a good commit message: https://cbea.ms/git-commit/\n\nSay what the commit does and why, please.\n\nPlease remove the \"Customer:\" part; this is an open source project and whilst the feature addition may have been prompted due to a customer, it\u0027s still just a feature being added to this open source project.",
      "parentUuid": "49d0bfa2_e1b8c956",
      "range": {
        "startLine": 6,
        "startChar": 0,
        "endLine": 8,
        "endChar": 0
      },
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": false,
      "key": {
        "uuid": "b0ead2bf_e47972b3",
        "filename": "/PATCHSET_LEVEL",
        "patchSetId": 2
      },
      "lineNbr": 0,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T12:58:06Z",
      "side": 1,
      "message": "Thanks for the patch.  However, it needs a little work to bring it up to mergeable status.  Please review the comments below (and the inline comments).\n\nAs the patch is changing an existing function that has test coverage (upload_signed_csr()) it really also need some additional unit test coverage.  This is to ensure that future changes to break this addition.  i.e. the unit tests validate the contract that the functional signature offers, and this is changing the function signature of upload_signed_csr().\n\nI\u0027m holding of a charm-recheck as the patch will require additional changes. The failure was due to an underlying infra issue, and not related to the patch. Thanks.",
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "93a24837_71b28a0a",
        "filename": "src/actions.yaml",
        "patchSetId": 2
      },
      "lineNbr": 110,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T13:00:49Z",
      "side": 1,
      "message": "Could this not be \"crl-distribution-point\" to more closely match the parameter in the vault call?",
      "range": {
        "startLine": 110,
        "startChar": 4,
        "endLine": 110,
        "endChar": 18
      },
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "c4a2db20_9b73746e",
        "filename": "src/actions.yaml",
        "patchSetId": 2
      },
      "lineNbr": 112,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T12:58:06Z",
      "side": 1,
      "message": "trailing whitespace needs removing.",
      "range": {
        "startLine": 112,
        "startChar": 0,
        "endLine": 112,
        "endChar": 18
      },
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "b322089b_4fee06a9",
        "filename": "src/lib/charm/vault_pki.py",
        "patchSetId": 2
      },
      "lineNbr": 206,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T12:58:06Z",
      "side": 1,
      "message": "need to add the :param ...: and :type ...: into the function\u0027s docstring to explain what the parameter is for, and the type s allowed.  Note the existing ones are wrong.\n\nIn this case it should be:\n\n    :pararm crl_dist_point: explain what it is\n    :type crl_dist_point: Optional[str]\n    \nBonus points if you update the existing types to include `Optional[\u003ctype\u003e]` for the optional parameters!",
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "d90ea1e8_e2babb72",
        "filename": "src/lib/charm/vault_pki.py",
        "patchSetId": 2
      },
      "lineNbr": 206,
      "author": {
        "id": 20870
      },
      "writtenOn": "2024-01-05T13:00:49Z",
      "side": 1,
      "message": "This param would probably be better named as \"crl_distribution_point\" to more closely match the calling param in set_urls.\n\nAlso is there a reason it is singular?  The set_urls() function appears to be expecting a list of urls?",
      "range": {
        "startLine": 206,
        "startChar": 41,
        "endLine": 206,
        "endChar": 42
      },
      "revId": "b2d1dddc27f8f71382a2ff68f56470b51dcdf986",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
    }
  ]
}