Update patch set 2

Patch Set 2: Code-Review-1

(4 comments)

Patch-set: 2
Reviewer: Gerrit User 20870 <20870@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1, 4ee324c8828f9a8e210e0840e5deb1e791acca52
This commit is contained in:
Gerrit User 20870 2024-01-05 12:58:06 +00:00 committed by Gerrit Code Review
parent edc2896148
commit 0f54d64055
1 changed files with 84 additions and 0 deletions

View File

@ -0,0 +1,84 @@
{
"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": 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": "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"
}
]
}