)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"698fa946_9337c99d","updated":"2024-02-29 23:35:14.000000000","message":"I think there is a bug in at least one of my comments.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"5bf5ebe13f232a421d44ed7b4c794168ef17c711","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"7e137de8_ed84a48a","updated":"2024-03-01 07:18:53.000000000","message":"recheck","commit_id":"171d4c56b1f1ee1f712b91f4ee466a9a37f9d298"}],"tests/fixtures/layouts/github-schema.yaml":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":24,"context_line":"          branch: ^master$"},{"line_number":25,"context_line":"        - event:"},{"line_number":26,"context_line":"            - pull_request"},{"line_number":27,"context_line":"            - check_run"},{"line_number":28,"context_line":"    success:"},{"line_number":29,"context_line":"      github: {}"},{"line_number":30,"context_line":"    failure:"}],"source_content_type":"text/x-yaml","patch_set":4,"id":"7404be17_e3122957","line":27,"updated":"2024-02-29 23:35:14.000000000","message":"No comment for this being deprecated now? It should produce a warning right?\n\nAdditionally should we have tests for unlabel, require-status, and requested?","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":24,"context_line":"          branch: ^master$"},{"line_number":25,"context_line":"        - event:"},{"line_number":26,"context_line":"            - pull_request"},{"line_number":27,"context_line":"            - check_run"},{"line_number":28,"context_line":"    success:"},{"line_number":29,"context_line":"      github: {}"},{"line_number":30,"context_line":"    failure:"}],"source_content_type":"text/x-yaml","patch_set":4,"id":"a7a996b2_2fc1e4e3","line":27,"in_reply_to":"7404be17_e3122957","updated":"2024-03-01 00:36:59.000000000","message":"Yes, and it is covered in the test.  If there were a comment here it would say \"as a list is deprecated\"\n\nI can add tests for the others.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"}],"zuul/driver/github/githubtrigger.py":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":66,"context_line":"            if \u0027require-status\u0027 in trigger:"},{"line_number":67,"context_line":"                with pcontext.confAttr(trigger, \u0027unlabel\u0027):"},{"line_number":68,"context_line":"                    pcontext.accumulator.addError("},{"line_number":69,"context_line":"                        GithubRequireStatusDeprecation())"},{"line_number":70,"context_line":"            # Deprecated with warning in 10.0"},{"line_number":71,"context_line":"            if \u0027unlabel\u0027 in trigger:"},{"line_number":72,"context_line":"                with pcontext.confAttr(trigger, \u0027unlabel\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"d6be2664_eee6da72","line":69,"updated":"2024-02-29 23:35:14.000000000","message":"This block checks if require-status is in trigger, but then enters the unlabel context. I think this is bad copy paste and this block needs to be fully updated for require-status.\n\nAlso a good reason to add the suggested tests in the config file comment I left.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":66,"context_line":"            if \u0027require-status\u0027 in trigger:"},{"line_number":67,"context_line":"                with pcontext.confAttr(trigger, \u0027unlabel\u0027):"},{"line_number":68,"context_line":"                    pcontext.accumulator.addError("},{"line_number":69,"context_line":"                        GithubRequireStatusDeprecation())"},{"line_number":70,"context_line":"            # Deprecated with warning in 10.0"},{"line_number":71,"context_line":"            if \u0027unlabel\u0027 in trigger:"},{"line_number":72,"context_line":"                with pcontext.confAttr(trigger, \u0027unlabel\u0027):"}],"source_content_type":"text/x-python","patch_set":4,"id":"f55ca1c5_8a07ff3c","line":69,"in_reply_to":"d6be2664_eee6da72","updated":"2024-03-01 00:36:59.000000000","message":"Yes, should fix; the test won\u0027t actually detect the error though; it will just point at the start of the trigger definition if it can\u0027t find the attribute.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":103,"context_line":"            # to the original action \u0027rerequested\u0027."},{"line_number":104,"context_line":"            # TODO: Remove after zuul 5.0"},{"line_number":105,"context_line":"            # Note the original backwards compat handling for this did"},{"line_number":106,"context_line":"            # not allow \u0027requested\u0027 as a list, so we don\u0027t do that"},{"line_number":107,"context_line":"            # here either."},{"line_number":108,"context_line":"            if trigger.get(\u0027action\u0027) \u003d\u003d \u0027requested\u0027:"},{"line_number":109,"context_line":"                trigger[\u0027action\u0027] \u003d \u0027rerequested\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"1edc6eca_a2e42e61","line":106,"updated":"2024-02-29 23:35:14.000000000","message":"I\u0027m not sure where I see this being enforced. The schema at the end of this file allows a str or list for action.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":103,"context_line":"            # to the original action \u0027rerequested\u0027."},{"line_number":104,"context_line":"            # TODO: Remove after zuul 5.0"},{"line_number":105,"context_line":"            # Note the original backwards compat handling for this did"},{"line_number":106,"context_line":"            # not allow \u0027requested\u0027 as a list, so we don\u0027t do that"},{"line_number":107,"context_line":"            # here either."},{"line_number":108,"context_line":"            if trigger.get(\u0027action\u0027) \u003d\u003d \u0027requested\u0027:"},{"line_number":109,"context_line":"                trigger[\u0027action\u0027] \u003d \u0027rerequested\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"4673ae45_807c2488","line":106,"in_reply_to":"1edc6eca_a2e42e61","updated":"2024-03-01 00:36:59.000000000","message":"This comment is referring to the code right here, not the schema.  The schema does indeed allow both things, but the original backwards compat code assumed that action would be a string, and only acted if it was a string.  That was a bug, so this movement of the code keeps bug compatability.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":139,"context_line":"def getNewSchema():"},{"line_number":140,"context_line":"    # For now, this is only used to raise deprecation errors if the"},{"line_number":141,"context_line":"    # syntax does not match.  This was added in 10.0.  When we\u0027re"},{"line_number":142,"context_line":"    # ready to enforce this, rename this method getSchema."},{"line_number":143,"context_line":"    base_schema \u003d vs.Schema({"},{"line_number":144,"context_line":"        vs.Required(\u0027event\u0027): vs.Any(\u0027pull_request\u0027,"},{"line_number":145,"context_line":"                                     \u0027pull_request_review\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"362408ac_845a5e1d","line":142,"updated":"2024-02-29 23:35:14.000000000","message":"Nit we also need to modify this new schema to disallow lists in some places when we stop allowing those fields to be a list (for example event).","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":139,"context_line":"def getNewSchema():"},{"line_number":140,"context_line":"    # For now, this is only used to raise deprecation errors if the"},{"line_number":141,"context_line":"    # syntax does not match.  This was added in 10.0.  When we\u0027re"},{"line_number":142,"context_line":"    # ready to enforce this, rename this method getSchema."},{"line_number":143,"context_line":"    base_schema \u003d vs.Schema({"},{"line_number":144,"context_line":"        vs.Required(\u0027event\u0027): vs.Any(\u0027pull_request\u0027,"},{"line_number":145,"context_line":"                                     \u0027pull_request_review\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"20be8ef4_91539546","line":142,"in_reply_to":"362408ac_845a5e1d","updated":"2024-03-01 00:36:59.000000000","message":"This schema does not allow event to be a list (lines 144-147 describe a choice among strings).","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":151,"context_line":""},{"line_number":152,"context_line":"    # Pull request"},{"line_number":153,"context_line":"    pull_request_base_schema \u003d base_schema.extend({"},{"line_number":154,"context_line":"        vs.Required(\u0027event\u0027): \u0027pull_request\u0027,"},{"line_number":155,"context_line":"        \u0027branch\u0027: scalar_or_list(vs.Any(ZUUL_REGEX, str)),"},{"line_number":156,"context_line":"    })"},{"line_number":157,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"0c579520_012d4068","line":154,"updated":"2024-02-29 23:35:14.000000000","message":"When we extend schemas and redefine entries like we do here for `vs.Required(\u0027event\u0027)` we\u0027re replacing the schema rules for that entry. Then we choose which rules we want to apply in validate right? we aren\u0027t creating some merge of the two rules?\n\nIf that is the case should we drop the `vs.Required(\u0027event\u0027)` entry above on line 144 to avoid confusion (we appear to override it in each of these special cases here and below)?","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":151,"context_line":""},{"line_number":152,"context_line":"    # Pull request"},{"line_number":153,"context_line":"    pull_request_base_schema \u003d base_schema.extend({"},{"line_number":154,"context_line":"        vs.Required(\u0027event\u0027): \u0027pull_request\u0027,"},{"line_number":155,"context_line":"        \u0027branch\u0027: scalar_or_list(vs.Any(ZUUL_REGEX, str)),"},{"line_number":156,"context_line":"    })"},{"line_number":157,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"60618311_2ead027f","line":154,"in_reply_to":"0c579520_012d4068","updated":"2024-03-01 00:36:59.000000000","message":"It\u0027s basically a dict merge, so yes, it overwrites event.\n\nIf you follow the validate method below, we validate progressively based on the event type.  We get much better errors by having voluptuous check that first.  I think this produces the best outcome short of implementing all the checks manually.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":204,"context_line":"    init_schema \u003d base_schema.extend({}, extra\u003dvs.ALLOW_EXTRA)"},{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    def validate(data):"},{"line_number":207,"context_line":"        # We this methoud could be a simple vs.Any() across all the"},{"line_number":208,"context_line":"        # possibilities, but sometimes the error messages are less"},{"line_number":209,"context_line":"        # intuitive (e.g., indicating that the action is wrong rather"},{"line_number":210,"context_line":"        # than that a user has added an attribute that is incompatible"}],"source_content_type":"text/x-python","patch_set":4,"id":"05112834_e909cbeb","line":207,"updated":"2024-02-29 23:35:14.000000000","message":"Nit if we\u0027re making a fixup patchset can we update this comment to read a bit more clearly? I think we want `# This method could be a simple...`","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":204,"context_line":"    init_schema \u003d base_schema.extend({}, extra\u003dvs.ALLOW_EXTRA)"},{"line_number":205,"context_line":""},{"line_number":206,"context_line":"    def validate(data):"},{"line_number":207,"context_line":"        # We this methoud could be a simple vs.Any() across all the"},{"line_number":208,"context_line":"        # possibilities, but sometimes the error messages are less"},{"line_number":209,"context_line":"        # intuitive (e.g., indicating that the action is wrong rather"},{"line_number":210,"context_line":"        # than that a user has added an attribute that is incompatible"}],"source_content_type":"text/x-python","patch_set":4,"id":"775892db_c64b841e","line":207,"in_reply_to":"05112834_e909cbeb","updated":"2024-03-01 00:36:59.000000000","message":"Done.","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"5991adc4fb424fb44bf5f31431eba4baaddbf79f","unresolved":true,"context_lines":[{"line_number":218,"context_line":"        if isinstance(event, str):"},{"line_number":219,"context_line":"            # First, make sure the common items are correct."},{"line_number":220,"context_line":"            init_schema(data)"},{"line_number":221,"context_line":"        event \u003d data.get(\u0027event\u0027)"},{"line_number":222,"context_line":"        action \u003d data.get(\u0027action\u0027)"},{"line_number":223,"context_line":"        if event \u003d\u003d \u0027pull_request\u0027:"},{"line_number":224,"context_line":"            if action \u003d\u003d \u0027comment\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"29eda4b5_574fb284","line":221,"updated":"2024-02-29 23:35:14.000000000","message":"This appears to be a redundant `event \u003d data.get(\u0027event\u0027)` line. \n\nAdditionally, event may still be a list right? Don\u0027t we need to loop over event if it is a list and do the checks below? Similar thing with action. Or are we hoping that people will get iterative errors and first fix the list problem then fix any deprecated event and action types?","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"5f204b9a564b446bf4414ab1a6362e90e609c5ff","unresolved":false,"context_lines":[{"line_number":218,"context_line":"        if isinstance(event, str):"},{"line_number":219,"context_line":"            # First, make sure the common items are correct."},{"line_number":220,"context_line":"            init_schema(data)"},{"line_number":221,"context_line":"        event \u003d data.get(\u0027event\u0027)"},{"line_number":222,"context_line":"        action \u003d data.get(\u0027action\u0027)"},{"line_number":223,"context_line":"        if event \u003d\u003d \u0027pull_request\u0027:"},{"line_number":224,"context_line":"            if action \u003d\u003d \u0027comment\u0027:"}],"source_content_type":"text/x-python","patch_set":4,"id":"5cbd0a18_d5682500","line":221,"in_reply_to":"29eda4b5_574fb284","updated":"2024-03-01 00:36:59.000000000","message":"I\u0027ll remove the redundant line.\n\nThe expectation is that folks will fix the list problem first, then see subsequent errors if any remain.  I don\u0027t think it makes sense to try to validate the stanza if it is a list (the whole problem with that is that it either works because it\u0027s simple, or it\u0027s nonsensical, so if there are any errors, we would probably throw way too many and they wouldn\u0027t make sense).","commit_id":"11128a9eaf2a48713d6ed7a3575a70f8007705f8"}]}