Update patch set 2
Patch Set 2: Code-Review-1
(7 comments)
Patch-set: 2
Reviewer: Gerrit User 15343 <15343@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1, 356b63a85f6856d5172f2973daaf296bb8ee4271
Attention: {"person_ident":"Gerrit User 36606 \u003c36606@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_15343\u003e replied on the change"}
This commit is contained in:
Gerrit User 15343
Gerrit Code Review
parent
5af32e1988
commit
77185d0ec4
|
|
@ -0,0 +1,153 @@
|
|||
{
|
||||
"comments": [
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "26a8aacb_b807a4da",
|
||||
"filename": "/COMMIT_MSG",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 7,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "Looks to be passing to me ;-)\n\nMight want to add a\n\n\u003e Related-Change: Ice9cc9fe68684563f18ee527996e5a4292230a96\n\nLooks like we didn\u0027t write up a bug for it.",
|
||||
"range": {
|
||||
"startLine": 7,
|
||||
"startChar": 0,
|
||||
"endLine": 7,
|
||||
"endChar": 12
|
||||
},
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": false,
|
||||
"key": {
|
||||
"uuid": "63ee59e3_5688a789",
|
||||
"filename": "/PATCHSET_LEVEL",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 0,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "Commit message needs updating, especially since we\u0027re fixing problems outside of just SLO downloads.\n\nWas the command_helpers change because of some user-reported problem we saw? Or was it more of a drive-by? Looks like it was a little incomplete: it fixed some overall account stat duplication (and pretty-printing of object meta), but now there\u0027s per-policy stats that get duplicated, like\n\n```\n$ swift stat\n Account: AUTH_test\n Containers: 1\n Objects: 1\n Bytes: 5951\n Containers in policy \"Default\": 1\n Objects in policy \"Default\": 1\n Bytes in policy \"Default\": 5951\n Content-Type: text/plain; charset\u003dutf-8\n X-Timestamp: 1711055839.79847\nX-Account-Storage-Policy-Default-Container-Count: 1\n X-Account-Storage-Policy-Default-Object-Count: 1\n X-Account-Storage-Policy-Default-Bytes-Used: 5951\n Accept-Ranges: bytes\n Vary: Accept\n X-Trans-Id: txed899ee1e2c24e5c8ae1d-006627feb0\n X-Openstack-Request-Id: txed899ee1e2c24e5c8ae1d-006627feb0\n```\n\nI\u0027ve got mixed feelings about how the storage policy casing changed, but I\u0027m not sure it matters much.",
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "f4643d70_1b24c8ff",
|
||||
"filename": "swiftclient/command_helpers.py",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 52,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "So the casing may have changed here, too...",
|
||||
"range": {
|
||||
"startLine": 52,
|
||||
"startChar": 26,
|
||||
"endLine": 52,
|
||||
"endChar": 36
|
||||
},
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "5155d8fa_a116d97d",
|
||||
"filename": "swiftclient/command_helpers.py",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 93,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "So these are still mixed case, but all the hard-coded ones below are lower-case...",
|
||||
"range": {
|
||||
"startLine": 93,
|
||||
"startChar": 42,
|
||||
"endLine": 93,
|
||||
"endChar": 52
|
||||
},
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "245b54d6_101fe5da",
|
||||
"filename": "swiftclient/service.py",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 461,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "Or maybe\n```\nif any(h in headers for h in bad_md5_headers):\n```\n?",
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "fd1e52f9_59009824",
|
||||
"filename": "swiftclient/service.py",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 1282,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "OK, so `headers` is always coming out of one of our `Connection`s... what type does _that_ return?\n\nThe [change to `resp_header_dict`](https://github.com/openstack/python-swiftclient/commit/ac2a60dc0d2932583e70406b6d456e6ae094c3dd#diff-4560a2922ced3123f02b6f3fc057a42d7c105f0de987db28d1a614b059c41167R741) that set this off makes me think it should already be a `CaseInsensitiveDict`. Is this just for tests? Is there any path by which it could be something _other_ than `dict` or `CaseInsensitiveDict`?\n\nI wonder whether there might be a simplification if we go back to including a `.lower()` in `resp_header_dict` but continue to use `CaseInsensitiveDict`...",
|
||||
"range": {
|
||||
"startLine": 1282,
|
||||
"startChar": 48,
|
||||
"endLine": 1282,
|
||||
"endChar": 55
|
||||
},
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
},
|
||||
{
|
||||
"unresolved": true,
|
||||
"key": {
|
||||
"uuid": "ae59b9ce_6f0074cc",
|
||||
"filename": "test/unit/test_service.py",
|
||||
"patchSetId": 2
|
||||
},
|
||||
"lineNbr": 255,
|
||||
"author": {
|
||||
"id": 15343
|
||||
},
|
||||
"writtenOn": "2024-04-23T18:52:30Z",
|
||||
"side": 1,
|
||||
"message": "I\u0027d maybe point at (our) `resp_header_dict` instead of `requests` here. If we change that implementation, then see this test fail, it\u0027d be nice to have that cluse that the test may need updating.",
|
||||
"range": {
|
||||
"startLine": 255,
|
||||
"startChar": 23,
|
||||
"endLine": 255,
|
||||
"endChar": 54
|
||||
},
|
||||
"revId": "3035d16657f83568e97b152c167139dd39c3888e",
|
||||
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
|
||||
}
|
||||
]
|
||||
}
|
||||
Loading…
Reference in New Issue