)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":6889,"name":"Fabien Boucher","email":"fboucher@redhat.com","username":"fabien-boucher"},"change_message_id":"8e2af6049214f29ad0ecdd753e29628b0723d38b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"7b2a7203_d951631a","updated":"2024-01-20 16:21:38.000000000","message":"recheck","commit_id":"27c4925744ad5729d3d129a47953dd33e6662ae9"}],"zuul/driver/gitlab/gitlabconnection.py":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"68964898760e73addac4da6b3e5f0d8873f9709c","unresolved":true,"context_lines":[{"line_number":155,"context_line":"            event.action \u003d \u0027opened\u0027"},{"line_number":156,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027merge\u0027:"},{"line_number":157,"context_line":"            event.action \u003d \u0027merged\u0027"},{"line_number":158,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and not body.get(\"changes\", {}):"},{"line_number":159,"context_line":"            event.action \u003d \u0027changed\u0027"},{"line_number":160,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and body[\"changes\"].get(\"labels\", {}):"},{"line_number":161,"context_line":"            event.action \u003d \u0027labeled\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"fcefd57a_fffb60f5","line":158,"updated":"2024-01-19 16:29:07.000000000","message":"What happens if a user edits the merge request description?  I suspect that we get:\n  action: update\n  changes: {...old and new description...}\n\nSince editing the MR description is used to add or remove Depends-On lines, users may be relying on that to trigger pipeline runs.\n\nThe way this is currently written, we would need to make a new fake action like labeled.  But we shouldn\u0027t have done that to start with; these triggers were intended to be very thin layers on top of the remote system (and this issue demonstrates why).  To match the original design and the Gerrit driver, we should have had the triggers specified as:\n\n  event: merge_request\n  action: update\n  changes: description\n\nSince we already have fake actions, I\u0027m not opposed to adding another one, so I think either approach would be okay for this (but at some point, we should consider migrating to the more explicit one).\n\nRegardless, assuming I\u0027m correct about the description edit (please check), I think we need to distinguish that case and still allow it.","commit_id":"c94a07f36abc8a0270cec6265934d92d02ab587a"},{"author":{"_account_id":6889,"name":"Fabien Boucher","email":"fboucher@redhat.com","username":"fabien-boucher"},"change_message_id":"30e16a42bdb5d12ab8265dac4a766d72f3b22a5d","unresolved":true,"context_lines":[{"line_number":155,"context_line":"            event.action \u003d \u0027opened\u0027"},{"line_number":156,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027merge\u0027:"},{"line_number":157,"context_line":"            event.action \u003d \u0027merged\u0027"},{"line_number":158,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and not body.get(\"changes\", {}):"},{"line_number":159,"context_line":"            event.action \u003d \u0027changed\u0027"},{"line_number":160,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and body[\"changes\"].get(\"labels\", {}):"},{"line_number":161,"context_line":"            event.action \u003d \u0027labeled\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"c9292a62_b0cccf99","line":158,"in_reply_to":"2f284666_965d99f3","updated":"2024-01-20 16:32:18.000000000","message":"I\u0027ve updated with a new patchset because I\u0027ve figured out that an \u0027update\u0027 action get the \u0027oldrev\u0027 attribute (only in case of code change) and this simplify the checking. Yes the previous patchset will cause trouble as you said.\n\nNote that I have the setup to validate the proposed change.","commit_id":"c94a07f36abc8a0270cec6265934d92d02ab587a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"4f0ed871049d9ef18d9a46522fae87c5611e7dd7","unresolved":true,"context_lines":[{"line_number":155,"context_line":"            event.action \u003d \u0027opened\u0027"},{"line_number":156,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027merge\u0027:"},{"line_number":157,"context_line":"            event.action \u003d \u0027merged\u0027"},{"line_number":158,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and not body.get(\"changes\", {}):"},{"line_number":159,"context_line":"            event.action \u003d \u0027changed\u0027"},{"line_number":160,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and body[\"changes\"].get(\"labels\", {}):"},{"line_number":161,"context_line":"            event.action \u003d \u0027labeled\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"2f284666_965d99f3","line":158,"in_reply_to":"fcefd57a_fffb60f5","updated":"2024-01-19 21:53:15.000000000","message":"I think the minimal form of this change would execute this branch if changes is empty or if it has whatever happens with a MR description edit.  Or perhaps a switch like:\nif labels in changes: labeled action\nif reviews in changes: review action (this is new)\nelse: changed action\n\nI think either of those would work without breaking anyone.","commit_id":"c94a07f36abc8a0270cec6265934d92d02ab587a"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"512fcd8eddb111b1c2881d8daf261025973abeb0","unresolved":true,"context_lines":[{"line_number":161,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and \"description\" in body[\"changes\"]:"},{"line_number":162,"context_line":"            event.merge_request_description_changed \u003d True"},{"line_number":163,"context_line":"            event.action \u003d \u0027changed\u0027"},{"line_number":164,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and body[\"changes\"].get(\"labels\"):"},{"line_number":165,"context_line":"            event.action \u003d \u0027labeled\u0027"},{"line_number":166,"context_line":"            previous_labels \u003d ["},{"line_number":167,"context_line":"                label[\"title\"] for"}],"source_content_type":"text/x-python","patch_set":4,"id":"8c833422_318304ae","line":164,"updated":"2024-01-23 15:17:48.000000000","message":"I\u0027m curious why the change from \"label in changes\" to \"changes.get(\u0027labels\u0027)\"?\n\nIt looks like changes.labels is always itself a dictionary, so if it exists, it should always evaluate to true, so we should end up with the same behavior.  But I did have to double check that since if there was a way for it to evaluate to false the behavior would change.","commit_id":"ff94910877e08b5736e5270d8a3a5bbaabdd2fd5"},{"author":{"_account_id":6889,"name":"Fabien Boucher","email":"fboucher@redhat.com","username":"fabien-boucher"},"change_message_id":"21dd0fb2e24a50504b654c57bb7f2377e821b2b5","unresolved":true,"context_lines":[{"line_number":161,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and \"description\" in body[\"changes\"]:"},{"line_number":162,"context_line":"            event.merge_request_description_changed \u003d True"},{"line_number":163,"context_line":"            event.action \u003d \u0027changed\u0027"},{"line_number":164,"context_line":"        elif attrs[\u0027action\u0027] \u003d\u003d \u0027update\u0027 and body[\"changes\"].get(\"labels\"):"},{"line_number":165,"context_line":"            event.action \u003d \u0027labeled\u0027"},{"line_number":166,"context_line":"            previous_labels \u003d ["},{"line_number":167,"context_line":"                label[\"title\"] for"}],"source_content_type":"text/x-python","patch_set":4,"id":"ae5611bd_2ee5061f","line":164,"in_reply_to":"8c833422_318304ae","updated":"2024-01-24 09:30:01.000000000","message":"Yes, this change is not related to the purpose of this patch. I you prefer I can revert that part to avoid unintended behavior change.\n\nHowever, when there is a change to MR\u0027s label then changes.labels looks like:\n\n\u0027\u0027\u0027\n{\u0027changes\u0027: {\u0027labels\u0027: {\u0027current\u0027: [{\u0027color\u0027: \u0027#dc143c\u0027, \u0027created_at\u0027: \u00272024-01-19 11:36:30 UTC\u0027, \u0027description\u0027: None, \u0027group_id\u0027: None, \u0027id\u0027: 105672, \u0027lock_on_merge\u0027: False, \u0027project_id\u0027: 84160, \u0027template\u0027: False, \u0027title\u0027\n: \u0027test\u0027, \u0027type\u0027: \u0027ProjectLabel\u0027, \u0027updated_at\u0027: \u00272024-01-19 11:36:30 UTC\u0027}], \u0027previous\u0027: []}}}\n\u0027\u0027\u0027\n\nIt seems that `labels` will always have the \u0027current\u0027 and \u0027previous\u0027 keys however I was looking to add a bit more safety here by ensuring the \u0027labels\u0027 is not an empty dict to avoid exception later where we expect the \u0027previous\u0027 and \u0027current\u0027 keys.\n\nIf \u0027labels\u0027 is {} (perhaps it will never) it will evaluate to false line 164.\n\nLet me know if you prefer a revert of that part.","commit_id":"ff94910877e08b5736e5270d8a3a5bbaabdd2fd5"}]}