)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"063aa33ddf930d6720167f836c9f80456e0a2e6c","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Jacob Anders \u003cjanders@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2022-11-03 21:59:51 +1000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Only use SettingsURI for BootSourceOverride if the attribute is present"},{"line_number":8,"context_line":"in GET response"},{"line_number":9,"context_line":""},{"line_number":10,"context_line":"Enhance handling of System Settings URI so that it is only used"},{"line_number":11,"context_line":"for PATCH requests needed for boot sequence overrides if"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"2c621018_eeaf10ce","line":8,"range":{"start_line":7,"start_character":0,"end_line":8,"end_character":15},"updated":"2022-11-04 09:04:02.000000000","message":"too long, maybe something like \u0027Fix setting boot related attributes\u0027","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"bb79324ac5d4770b8f62e9bd9cce7f3ff88bc7aa","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Jacob Anders \u003cjanders@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2022-11-03 21:59:51 +1000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Only use SettingsURI for BootSourceOverride if the attribute is present"},{"line_number":8,"context_line":"in GET response"},{"line_number":9,"context_line":""},{"line_number":10,"context_line":"Enhance handling of System Settings URI so that it is only used"},{"line_number":11,"context_line":"for PATCH requests needed for boot sequence overrides if"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"6a14243e_7da9c809","line":8,"range":{"start_line":7,"start_character":0,"end_line":8,"end_character":15},"in_reply_to":"2c621018_eeaf10ce","updated":"2022-11-07 12:45:33.000000000","message":"Done","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"8fed241b28280cd93b65023f8d00383a609f94c6","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"ab9a58a1_b36e401c","updated":"2022-09-30 14:05:08.000000000","message":"Hi,\n\nnot sure that the issue is that Settings URI is read-only, but some of the properties, in this case, BootSourceOverrideTarget and BootSourceOverrideEnabled, need to be updated directly as per Redfish spec.\nThis seems to be conflicting with previous change for Nokia systems that couldn\u0027t update BootSourceOverrideTarget directly [1][2].\n\nIn Redfish spec [3], section 9.10 Settings resource, Table 23 (p. 105), 3rd row it describes \"Writable, updates immediately, but not at a future point in time.\" - \"Not returned in the settings resource response to a GET request. *Modification requests are rejected*\" which is happening here. I don\u0027t have access to Lenovo systems, but for iDRAC these 2 properties are applied immediately and will be rejected by Settings resource.\n\nThe proposed solution works, but describes this as a bug while I see it complying with the spec. Ideally, sushy would be able to tell which properties are modified on active resource and which on pending resource and that way avoid retrying on failure. Not clear if sets are different among vendors.\n\nLeaving this here for further discussion and to gather ideas.\n\n\n[1] https://storyboard.openstack.org/#!/story/2009871\n[2] https://review.opendev.org/c/openstack/sushy/+/830553\n[3] https://www.dmtf.org/sites/default/files/standards/documents/DSP0266_1.16.0.pdf","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"4e4209b306a669949359d9ff081e71a89afd311c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"82005a37_744390b9","in_reply_to":"0b60dd89_e0509e54","updated":"2022-11-03 02:09:59.000000000","message":"Hi Aija. I re-factored the patch along the lines of what you suggested. Please have a look and let me know what you think.\nIt passes tests, I also tested it on a Lenovo SE450 and it seems to resolve the problem.\nThe change breaks one test: test_set_system_boot_options_nothing_specified()  which doesn\u0027t seem critical:\nhttps://review.opendev.org/c/openstack/sushy/+/856597/19/sushy/tests/unit/resources/system/test_system.py#317\nI\u0027mm not sure if it\u0027s worth fixing, or should it be removed? PATCH with an empty body does not seem particularly useful.","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"b0a53c8740f5fce626f27d7b89f35a61942ac9dc","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"c45d27e1_8fd04a22","in_reply_to":"66a9c487_bb443829","updated":"2022-10-06 11:30:56.000000000","message":"Further to the above and the IRC discussion here is a GET of Nokia SD resource (Nokia machines also use Settings URI). This may inform decisions about future direction of this patch: https://paste.openstack.org/show/bOGB1mJ3D2ZrStf2siUE/","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"063aa33ddf930d6720167f836c9f80456e0a2e6c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"45e6724b_53d6e0a6","in_reply_to":"82005a37_744390b9","updated":"2022-11-04 09:04:02.000000000","message":"as discussed on IRC, can keep this test and adjust the expected outcome to not have any PATCHes requested. For me it is unclear why original change was PATCHing with empty body instead of not PATCHing at all, but maybe I don\u0027t know something.","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"5af3567130d27f6a0feb3e7092f77aec80605f7a","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"66a9c487_bb443829","in_reply_to":"ab9a58a1_b36e401c","updated":"2022-10-03 23:35:16.000000000","message":"Thank you for your reviews Aija. You shared some really valuable insights which are very much appreciated.\n\nI will do some more research on this topic, focusing in particular on the responses from Nokia and Lenovo servers and how they relate to the part of the specifications that you referenced.\n\nI may re-work this patch if/as required and post further responses.\n\nThanks again!","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"590e3088a6b02fbe57348835ffe640923f5f5e00","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"fad8d43e_93b7668c","in_reply_to":"c45d27e1_8fd04a22","updated":"2022-10-26 05:37:38.000000000","message":"Thank you for your comprehensive response and your suggestions, Aija. I reworked the patch along the lines of your recommendations. Please let me know what you think about the new approach.\n\nI have one outstanding question though. With Lenovo SE450, I see Boot attribute in SettingsURI but not BootSourceOverrideTarget or BootSourceOverrideEnabled. This itself does not worry me but raises a question: Do we need to worry about cases where BootSourceOverrideTarget is present but BootSourceOverrideEnabled is absent from SettingsURI GET response?\n\nFor now I am only checking for BootSourceOverrideTarget as without it we certainly won\u0027t be able to do anything useful, but I wonder if this is sufficient. Please see my in-code comment below:\n\nhttps://review.opendev.org/c/openstack/sushy/+/856597/14/sushy/resources/system/system.py#288","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"320f10a53844a9b6324e2152a3a25cffc33c1a3b","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"0b60dd89_e0509e54","in_reply_to":"c8fbd719_fc738132","updated":"2022-10-26 13:04:38.000000000","message":"Thank you Aija.\n\nLooking at https://opendev.org/openstack/sushy/src/branch/master/sushy/resources/system/system.py#L215 (set_system_boot_options()) I think there are only three properties that may be used: BootSourceOverrideTarget, BootSourceOverrideEnabled and BootSourceOverrideMode.\n\nFor my understanding, there is no point of making a request to the Settings Resource if it doesn\u0027t have the BootSourceOverrideTarget property (hence the current conditional - if that\u0027s missing I think fallback to System URI is appropriate).\n\nNow - hypothetically, what if Settings Resource has BootSourceOverrideTarget but not BootSourceOverrideEnabled? I don\u0027t think having one of these in Settings Resource and the other one under System would be a reasonable implementation (in which I\u0027d disregard it for now)? Or do you think we need to consider such scenario?\n\nIn my experience, hardware using Settings Resource for boot management is relatively scarce, I only worked with one machine that manages boot through Settings Resource. This makes it hard to predict what other combination may pop up.\n\nI agree that we may re-think this at some point when/if Settings Resource becomes more widespread but I\u0027d be tempted to try to fix the current problem with the Lenovo SE450 and re-visit this in the future if needed.\n\nWhat do you think about this approach?","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"48b67c78b3ede407817d2ab7f2181c388a2761a6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"c8fbd719_fc738132","in_reply_to":"fad8d43e_93b7668c","updated":"2022-10-26 12:53:14.000000000","message":"I\u0027d check all properties that will be used and group them by path. In the end there could be 2 requests - one for active resource, another for settings resource.\n\nIn iDRAC current approach would make BootSourceOverrideMode to be patched on active resource that will still work as iDRAC has kept backward compatibility. Could argue that need to change BootSourceOverrideMode will go away as there will be less and less system with legacy BIOS support, but I see that it is safer to check what\u0027s described in settings/pending resource than on observation of current behavior of known BMCs.","commit_id":"e6d40325c2f533ad580dcfdbd032b9015beed0cf"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"fa0064a341ed0ad3f40c265fed0d07d18332a673","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"6e09c18c_158b0102","updated":"2022-10-26 13:08:53.000000000","message":"Thank you for your comment Dmitry, uploading a revised version.","commit_id":"edf9208025be30933ea7204674552c57b66e1dc7"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"fc6cdbb83adbde5d0a4eeaea64eaf890c89e8616","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":17,"id":"6283ee68_82b90257","updated":"2022-10-31 12:11:32.000000000","message":"finished the rebase work, still need to refactor the code along the lines of what we discussed with Dmitry and Aija - setting W-1","commit_id":"a0a6c56dafd4e857a39026ac1136fef1976c3ab1"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"bb79324ac5d4770b8f62e9bd9cce7f3ff88bc7aa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"b52ac7c3_f35474df","updated":"2022-11-07 12:45:33.000000000","message":"Thank you for the subsequent review and helpful pointers, Aija. Submitting a revised version - let me know what you think.","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"063aa33ddf930d6720167f836c9f80456e0a2e6c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"a7c53851_2c5556f4","updated":"2022-11-04 09:04:02.000000000","message":"Thanks for the updates, looks good, some smaller comments inline.","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"aec46beefaeb43daefd4be836cfc4991d0a43e6f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":21,"id":"1c51e237_4b6fc91d","updated":"2022-11-09 10:14:02.000000000","message":"thanks, LGTM.\n\nThere is some repetition when choosing which resource to use, maybe in future could abstract that if more occurrences in sushy show up for this pattern.\n","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"e8cbf058317ba6abcb7bf40fe455136473daa2b4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":22,"id":"466f5468_0f7fcaff","updated":"2022-11-09 11:46:52.000000000","message":"Thank you Aija, I tried addressing the minor issues, let me know if you\u0027re happy with the changes.","commit_id":"14c4bdae7de20bd2b45f470c5aba2f6e5754bda3"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"1af083d185591112755ef585f3bee916718cac14","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"ce13713e_1f0cc67b","updated":"2022-11-11 05:13:28.000000000","message":"Thank you for suggestions Dmitry and Aija. Should be all resolved now.","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"d230ab995d4fc631a8c8b54880fc40d31f383de9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":25,"id":"fc2f954a_11f09c7e","updated":"2022-11-22 07:45:06.000000000","message":"Thanks","commit_id":"2c8229046c6ca8aa342e028c96aaa066f517eb9f"}],"releasenotes/notes/check-for-boot-attrs-in-settingsuri-1cad07b6eb1c81b3.yaml":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"aec46beefaeb43daefd4be836cfc4991d0a43e6f","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Adds an extra check for cases where the BMC provides a SettingsObject"},{"line_number":5,"context_line":"    URI through @Redfish.Settings but this URI does not allow setting boot"},{"line_number":6,"context_line":"    related attributes. Prior to sending a PATCH request to SettingsURI, a"}],"source_content_type":"text/x-yaml","patch_set":21,"id":"1a667696_e60a2a0c","line":3,"updated":"2022-11-09 10:14:02.000000000","message":"Nice to have: some sample error message that users would get when running without this fix so that they can match the error with this fix.","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"e8cbf058317ba6abcb7bf40fe455136473daa2b4","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Adds an extra check for cases where the BMC provides a SettingsObject"},{"line_number":5,"context_line":"    URI through @Redfish.Settings but this URI does not allow setting boot"},{"line_number":6,"context_line":"    related attributes. Prior to sending a PATCH request to SettingsURI, a"}],"source_content_type":"text/x-yaml","patch_set":21,"id":"645d9d3f_41c31205","line":3,"in_reply_to":"1a667696_e60a2a0c","updated":"2022-11-09 11:46:52.000000000","message":"Done","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"b856a51a363abeb43ad8f974eb50e272c601e471","unresolved":true,"context_lines":[{"line_number":8,"context_line":"    updated. In case these attributes are missing, the request is made against"},{"line_number":9,"context_line":"    System URI instead."},{"line_number":10,"context_line":"    Issues such as the one addressed with this change usually manifest"},{"line_number":11,"context_line":"    themselves with a RedFish response containing an error message similar to"},{"line_number":12,"context_line":"    the following: ``MessageId: Base.1.8.PropertyNotWritable, Message: The"},{"line_number":13,"context_line":"    property BootSourceOverrideEnabled is a read only property and cannot be"},{"line_number":14,"context_line":"    assigned a value.``"}],"source_content_type":"text/x-yaml","patch_set":23,"id":"c50d0c7f_56ae51f2","line":11,"range":{"start_line":11,"start_character":22,"end_line":11,"end_character":29},"updated":"2022-11-10 08:16:13.000000000","message":"nit: DMTF Redfish documentation does not use this casing, instead: Redfish.","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"1af083d185591112755ef585f3bee916718cac14","unresolved":false,"context_lines":[{"line_number":8,"context_line":"    updated. In case these attributes are missing, the request is made against"},{"line_number":9,"context_line":"    System URI instead."},{"line_number":10,"context_line":"    Issues such as the one addressed with this change usually manifest"},{"line_number":11,"context_line":"    themselves with a RedFish response containing an error message similar to"},{"line_number":12,"context_line":"    the following: ``MessageId: Base.1.8.PropertyNotWritable, Message: The"},{"line_number":13,"context_line":"    property BootSourceOverrideEnabled is a read only property and cannot be"},{"line_number":14,"context_line":"    assigned a value.``"}],"source_content_type":"text/x-yaml","patch_set":23,"id":"739007b2_e8eae6de","line":11,"range":{"start_line":11,"start_character":22,"end_line":11,"end_character":29},"in_reply_to":"c50d0c7f_56ae51f2","updated":"2022-11-11 05:13:28.000000000","message":"Done","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"}],"sushy/resources/system/system.py":[{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"9a9a3e8c6a796c02264091c94daaa6faed12421f","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            # Obviously we can\u0027t do anything useful without"},{"line_number":292,"context_line":"            # BootSourceOverrideTarget so will start with just checking for it"},{"line_number":293,"context_line":"            # but this logic may need expanding futher."},{"line_number":294,"context_line":"            if resp.json().get(\u0027Boot\u0027).get(\u0027BootSourceOverrideTarget\u0027):"},{"line_number":295,"context_line":"                # NOTE(janders): if a machine supports SettingsURI AND"},{"line_number":296,"context_line":"                # the attribute we are trying to say is listed in it, then"},{"line_number":297,"context_line":"                # we use it (along with matching eTag if sent through),"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a093773_90d44681","line":294,"updated":"2022-10-26 11:13:40.000000000","message":"The first get() can return None, you probably meant get(\u0027Boot\u0027, {})","commit_id":"edf9208025be30933ea7204674552c57b66e1dc7"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"fa0064a341ed0ad3f40c265fed0d07d18332a673","unresolved":false,"context_lines":[{"line_number":291,"context_line":"            # Obviously we can\u0027t do anything useful without"},{"line_number":292,"context_line":"            # BootSourceOverrideTarget so will start with just checking for it"},{"line_number":293,"context_line":"            # but this logic may need expanding futher."},{"line_number":294,"context_line":"            if resp.json().get(\u0027Boot\u0027).get(\u0027BootSourceOverrideTarget\u0027):"},{"line_number":295,"context_line":"                # NOTE(janders): if a machine supports SettingsURI AND"},{"line_number":296,"context_line":"                # the attribute we are trying to say is listed in it, then"},{"line_number":297,"context_line":"                # we use it (along with matching eTag if sent through),"}],"source_content_type":"text/x-python","patch_set":14,"id":"cc0d7260_2191b953","line":294,"in_reply_to":"5a093773_90d44681","updated":"2022-10-26 13:08:53.000000000","message":"Good pickup Dmitry, fixed now.","commit_id":"edf9208025be30933ea7204674552c57b66e1dc7"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"063aa33ddf930d6720167f836c9f80456e0a2e6c","unresolved":true,"context_lines":[{"line_number":290,"context_line":"                    valid_values\u003dlist(sys_cons.BootSourceOverrideMode))"},{"line_number":291,"context_line":"            if (settings_resp and settings_resp.json()"},{"line_number":292,"context_line":"                    .get(\u0027Boot\u0027, {}).get(\u0027BootSourceOverrideMode\u0027)):"},{"line_number":293,"context_line":"                settings_data[\u0027Boot\u0027][\u0027BootSourceOverrideMode\u0027] \u003d fishy_mode"},{"line_number":294,"context_line":"            else:"},{"line_number":295,"context_line":"                data[\u0027Boot\u0027][\u0027BootSourceOverrideMode\u0027] \u003d fishy_mode"},{"line_number":296,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"c31df16d_4b60e003","line":293,"updated":"2022-11-04 09:04:02.000000000","message":"this line is missing unit test coverage. Could check if existing test could be adjusted or new one is needed. This is realistic scenario for iDRAC systems - BootSourceOverrideMode out of 3 attributes will be the only one in Settings object.","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"bb79324ac5d4770b8f62e9bd9cce7f3ff88bc7aa","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                    valid_values\u003dlist(sys_cons.BootSourceOverrideMode))"},{"line_number":291,"context_line":"            if (settings_resp and settings_resp.json()"},{"line_number":292,"context_line":"                    .get(\u0027Boot\u0027, {}).get(\u0027BootSourceOverrideMode\u0027)):"},{"line_number":293,"context_line":"                settings_data[\u0027Boot\u0027][\u0027BootSourceOverrideMode\u0027] \u003d fishy_mode"},{"line_number":294,"context_line":"            else:"},{"line_number":295,"context_line":"                data[\u0027Boot\u0027][\u0027BootSourceOverrideMode\u0027] \u003d fishy_mode"},{"line_number":296,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"0db0dc28_962bec4a","line":293,"in_reply_to":"c31df16d_4b60e003","updated":"2022-11-07 12:45:33.000000000","message":"Added test_set_system_boot_options_settings_resource_bootmode_only() - let me know if you think it\u0027s sufficient.","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"628da1672c913c39fa8a8c8bdad982663f3b95b6","unresolved":true,"context_lines":[{"line_number":231,"context_line":"        settings_data \u003d collections.defaultdict(dict)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"        if self._settings and self._settings.resource_uri:"},{"line_number":234,"context_line":"            settings_resp \u003d self._conn.get(self._settings.resource_uri)"},{"line_number":235,"context_line":"            settings_path \u003d self._settings.resource_uri"},{"line_number":236,"context_line":"            settings_etag \u003d settings_resp.headers.get(\u0027ETag\u0027)"},{"line_number":237,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":23,"id":"5e96e613_9f9e2712","line":234,"updated":"2022-11-09 14:44:14.000000000","message":"I\u0027d prefer we parse the JSON here rather than 3 times below. Maybe even do the get(\u0027Boot\u0027, {}) part as well.","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"b856a51a363abeb43ad8f974eb50e272c601e471","unresolved":true,"context_lines":[{"line_number":231,"context_line":"        settings_data \u003d collections.defaultdict(dict)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"        if self._settings and self._settings.resource_uri:"},{"line_number":234,"context_line":"            settings_resp \u003d self._conn.get(self._settings.resource_uri)"},{"line_number":235,"context_line":"            settings_path \u003d self._settings.resource_uri"},{"line_number":236,"context_line":"            settings_etag \u003d settings_resp.headers.get(\u0027ETag\u0027)"},{"line_number":237,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":23,"id":"62c443ae_0d4e5453","line":234,"in_reply_to":"5e96e613_9f9e2712","updated":"2022-11-10 08:16:13.000000000","message":"+1","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"1af083d185591112755ef585f3bee916718cac14","unresolved":false,"context_lines":[{"line_number":231,"context_line":"        settings_data \u003d collections.defaultdict(dict)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"        if self._settings and self._settings.resource_uri:"},{"line_number":234,"context_line":"            settings_resp \u003d self._conn.get(self._settings.resource_uri)"},{"line_number":235,"context_line":"            settings_path \u003d self._settings.resource_uri"},{"line_number":236,"context_line":"            settings_etag \u003d settings_resp.headers.get(\u0027ETag\u0027)"},{"line_number":237,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":23,"id":"ceb2d658_7b6d4b6b","line":234,"in_reply_to":"62c443ae_0d4e5453","updated":"2022-11-11 05:13:28.000000000","message":"Done","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"628da1672c913c39fa8a8c8bdad982663f3b95b6","unresolved":true,"context_lines":[{"line_number":302,"context_line":"        if settings_data.get(\u0027Boot\u0027):"},{"line_number":303,"context_line":"            self._conn.patch(settings_path, data\u003dsettings_data,"},{"line_number":304,"context_line":"                             etag\u003dsettings_etag)"},{"line_number":305,"context_line":"        if data.get(\u0027Boot\u0027):"},{"line_number":306,"context_line":"            self._conn.patch(path, data\u003ddata, etag\u003detag)"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"    # TODO(etingof): we should remove this method, eventually"}],"source_content_type":"text/x-python","patch_set":23,"id":"c8e57493_ae728f9b","line":305,"updated":"2022-11-09 14:44:14.000000000","message":"I wonder if the previous call can change the etag... maybe it\u0027s too crazy for now :)","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"1af083d185591112755ef585f3bee916718cac14","unresolved":false,"context_lines":[{"line_number":302,"context_line":"        if settings_data.get(\u0027Boot\u0027):"},{"line_number":303,"context_line":"            self._conn.patch(settings_path, data\u003dsettings_data,"},{"line_number":304,"context_line":"                             etag\u003dsettings_etag)"},{"line_number":305,"context_line":"        if data.get(\u0027Boot\u0027):"},{"line_number":306,"context_line":"            self._conn.patch(path, data\u003ddata, etag\u003detag)"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"    # TODO(etingof): we should remove this method, eventually"}],"source_content_type":"text/x-python","patch_set":23,"id":"f66bdd4c_bb57b584","line":305,"in_reply_to":"c8e57493_ae728f9b","updated":"2022-11-11 05:13:28.000000000","message":"Done","commit_id":"97cb69ecdc1b49cddcb4de46bdf233a6445f0d35"}],"sushy/tests/unit/json_samples/readonly_settings_uri_error.json":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"063aa33ddf930d6720167f836c9f80456e0a2e6c","unresolved":true,"context_lines":[{"line_number":1,"context_line":"{"},{"line_number":2,"context_line":"  \"error\": {"},{"line_number":3,"context_line":"    \"code\": \"Base.1.8.GeneralError\","},{"line_number":4,"context_line":"    \"@Message.ExtendedInfo\": ["}],"source_content_type":"application/json","patch_set":20,"id":"f068d936_9924e5f2","line":1,"updated":"2022-11-04 09:04:02.000000000","message":"Is this file still used in the latest version?","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"bb79324ac5d4770b8f62e9bd9cce7f3ff88bc7aa","unresolved":false,"context_lines":[{"line_number":1,"context_line":"{"},{"line_number":2,"context_line":"  \"error\": {"},{"line_number":3,"context_line":"    \"code\": \"Base.1.8.GeneralError\","},{"line_number":4,"context_line":"    \"@Message.ExtendedInfo\": ["}],"source_content_type":"application/json","patch_set":20,"id":"42e63588_cf1ffd6e","line":1,"in_reply_to":"f068d936_9924e5f2","updated":"2022-11-07 12:45:33.000000000","message":"Done","commit_id":"649d8079ef4d72212f5d1440b443f35d1f39dab4"}],"sushy/tests/unit/json_samples/settings-lenovo-se450.json":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"aec46beefaeb43daefd4be836cfc4991d0a43e6f","unresolved":true,"context_lines":[{"line_number":9,"context_line":"    \"SettingsObject\": {"},{"line_number":10,"context_line":"      \"@odata.id\": \"/redfish/v1/Systems/1/Pending\""},{"line_number":11,"context_line":"    }"},{"line_number":12,"context_line":"\t}"},{"line_number":13,"context_line":"}"}],"source_content_type":"application/json","patch_set":21,"id":"582f31c3_978b403b","line":12,"range":{"start_line":12,"start_character":1,"end_line":12,"end_character":2},"updated":"2022-11-09 10:14:02.000000000","message":"nit: indentation of }","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"e8cbf058317ba6abcb7bf40fe455136473daa2b4","unresolved":false,"context_lines":[{"line_number":9,"context_line":"    \"SettingsObject\": {"},{"line_number":10,"context_line":"      \"@odata.id\": \"/redfish/v1/Systems/1/Pending\""},{"line_number":11,"context_line":"    }"},{"line_number":12,"context_line":"\t}"},{"line_number":13,"context_line":"}"}],"source_content_type":"application/json","patch_set":21,"id":"0f037beb_372e0d73","line":12,"range":{"start_line":12,"start_character":1,"end_line":12,"end_character":2},"in_reply_to":"582f31c3_978b403b","updated":"2022-11-09 11:46:52.000000000","message":"Done","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"e8cbf058317ba6abcb7bf40fe455136473daa2b4","unresolved":false,"context_lines":[{"line_number":9,"context_line":"    \"SettingsObject\": {"},{"line_number":10,"context_line":"      \"@odata.id\": \"/redfish/v1/Systems/1/Pending\""},{"line_number":11,"context_line":"    }"},{"line_number":12,"context_line":"\t}"},{"line_number":13,"context_line":"}"}],"source_content_type":"application/json","patch_set":21,"id":"a1b41615_7f266d73","line":12,"range":{"start_line":12,"start_character":1,"end_line":12,"end_character":2},"in_reply_to":"582f31c3_978b403b","updated":"2022-11-09 11:46:52.000000000","message":"Done","commit_id":"abcd0a21bfa183d51778c398bb2e00e130801c90"}],"sushy/tests/unit/resources/system/test_system.py":[{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"a79b1f8a5bd70a1d505864671867810213f9711e","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"e04fa10b_2b4ac1e9","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"updated":"2022-11-11 08:34:18.000000000","message":"As unit test mocks the same ETag for all requests, then it does not really test that if/else block where ETag values differ based on path. Up to you if want to improve on this.","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"f17dd17677e8f7cca71bd7885c6052f7e2f26e27","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"1cbcd164_497270ae","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"0840c3ab_2f2d8a62","updated":"2022-11-17 08:12:50.000000000","message":"No need for separate tests. To mock different return values, can use `side_effect`. It\u0027s up to you if you want to add such detail.","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"1048552251e58e9dfe86c586adb6e864a0e73b79","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"aa2da788_77bb4bb5","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"1cbcd164_497270ae","updated":"2022-11-17 16:56:50.000000000","message":"Yeah, I think side_effect can help.","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"4284fdeffdf6b53f7f4ae8f157d658f2088314fa","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"f85789a8_9c85f0f7","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"aa2da788_77bb4bb5","updated":"2022-11-18 10:04:26.000000000","message":"Thanks Aija and Dmitry. \n\nCan I please ask you for a couple of pointers how am I supposed to use the `side_effect` here? In particular what does this `side_effect` need to correspond to?\n\n1) GET request of the SettingsURI\n2) self._get_etag()\n3) something else (what)?\nAlso do I need to mock a GET request and make it a parameter of the test to make this work? Basic pseudocode or examples in existing code would be very helpful. I\u0027m hitting a blank trying to visualise it cause so far I\u0027ve only been explicitly setting eTags like it\u0027s done above.\n\nThanks in advance!","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"e58c6fe06c1822a1741eaa51025c403c84607d3b","unresolved":false,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"65a38006_f190cde5","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"b2400c86_59a2e84c","updated":"2022-11-22 00:32:55.000000000","message":"Updated the tests following the advice provided. Should be good to go. Thanks again Aija!","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"4408152b6abd15ec166ef3009fe2f042d71cc98f","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"b2400c86_59a2e84c","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"c4483a27_29e4495e","updated":"2022-11-21 12:42:44.000000000","message":"Thank you so much Aija, I will give it a crack tomorrow and report back. I really appreciate your help!","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":32177,"name":"Jacob Anders","email":"jacob-anders-dev@proton.me","username":"janders"},"change_message_id":"89151dc1c621cf78e8ff52558b2fc64ade697f92","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"0840c3ab_2f2d8a62","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"e04fa10b_2b4ac1e9","updated":"2022-11-17 07:11:07.000000000","message":"Thank you Aija. \n\nI had a bit of a look into this. I suppose I could have two different sets of headers in json corresponding to each response variant and make sure we load the response to the GET request against settings together with the right header, but thinking about the flow I\u0027m not sure if it will test much. Additionally, the assert already tests the path which is set in the same conditional as eTag so if that passes it\u0027s unlikely that the eTag will be incorrect - or at least that\u0027s my (possibly naive) thinking?\n\nLooking at other tests against System, all the tests I\u0027ve seen operate with a single self.conn.get so I am unsure how to deal with a case where we may have two GETs - one against the System and one against the Settings which we\u0027re doing here. Or at least not without adding a fair bit of complexity.\n\nI suppose we could have a new separate set of tests that just focus on making sure the right eTag is used and that would inject different Settings responses and monitor how many GET requests to we see and to what URLs (assert against the GETs not the PATCH)? However if we were to do this personally I\u0027d leave this for a follow-up change.\n\nLet me know your thoughts - perhaps I missed a really simple way of sorting this out (if that\u0027s the case I\u0027m open to addressing this within this change)\n\nThank you!","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"},{"author":{"_account_id":27909,"name":"Aija Jauntēva","email":"code@clusums.eu","username":"ajya"},"change_message_id":"e058ae395ac910084c8bf75620f068597cc162a6","unresolved":true,"context_lines":[{"line_number":449,"context_line":"            redfish_version\u003d\u00271.0.2\u0027)"},{"line_number":450,"context_line":""},{"line_number":451,"context_line":"        self.conn.get \u003d mock.MagicMock()"},{"line_number":452,"context_line":"        self.conn.get.return_value.headers \u003d {\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027}"},{"line_number":453,"context_line":"        with open(\u0027sushy/tests/unit/json_samples/\u0027"},{"line_number":454,"context_line":"                  \u0027settings-nokia.json\u0027) as f:"},{"line_number":455,"context_line":"            settings_obj \u003d json.load(f)"}],"source_content_type":"text/x-python","patch_set":24,"id":"c4483a27_29e4495e","line":452,"range":{"start_line":452,"start_character":7,"end_line":452,"end_character":73},"in_reply_to":"f85789a8_9c85f0f7","updated":"2022-11-21 11:58:30.000000000","message":"Replace ln 452, ln 464 with something like:\n\n  get_settings \u003d mock.MagicMock(headers\u003d{\u0027ETag\u0027: \u0027\"3d7b838291941d\"\u0027})\n  get_settings.json.return_value \u003d settings_obj\n  get_system \u003d mock.MagicMock(headers\u003d{\u0027ETag\u0027: \u0027\"222\"\u0027})\n  self.conn.get.side_effect\u003d[get_settings, get_system]\n  \nMaybe there is a way how to make it shorter, but the thing is that need to mock both `headers` field and `json` method.\n+ update other affected tests that assert etag to have different values to test both for system and settings URI.","commit_id":"d814412087e7e6b2064c8d9aef81230f5ce14202"}]}