)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"381e8cb944b03cec8d2607a5e0e1ecf48e497d5b","unresolved":false,"context_lines":[{"line_number":25,"context_line":"   negative, we raise."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"3) To investigate, but I don\u0027t think the \u0027validations_passed\u0027 variable"},{"line_number":28,"context_line":"   introduced in Ibc458d3badb807a0deef5a799db6682d7128dbc1 really makes"},{"line_number":29,"context_line":"   sense. It doesn\u0027t validate anything and moreover we were skipping"},{"line_number":30,"context_line":"   containers which failed that \"validation\" (!!!)."},{"line_number":31,"context_line":"   In this patch, I commented the variables, suggesting we should change"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_db92ed9f","line":28,"updated":"2019-08-09 09:33:04.000000000","message":"My thinking is that this patch didn\u0027t change the behavior. paunch\u0027s behavior up to date was that it runs a set of commands for a set of containers and only logs the errors - see my previous comment (base.py:134).","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"381e8cb944b03cec8d2607a5e0e1ecf48e497d5b","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":"3) To investigate, but I don\u0027t think the \u0027validations_passed\u0027 variable"},{"line_number":28,"context_line":"   introduced in Ibc458d3badb807a0deef5a799db6682d7128dbc1 really makes"},{"line_number":29,"context_line":"   sense. It doesn\u0027t validate anything and moreover we were skipping"},{"line_number":30,"context_line":"   containers which failed that \"validation\" (!!!)."},{"line_number":31,"context_line":"   In this patch, I commented the variables, suggesting we should change"},{"line_number":32,"context_line":"   them and also converted the \"continue\" into a \"raise\", and increased"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_bb3e11b7","line":29,"updated":"2019-08-09 09:33:04.000000000","message":"It validates volumes in docker and logs the error in the same way podman would behave in this case.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"381e8cb944b03cec8d2607a5e0e1ecf48e497d5b","unresolved":false,"context_lines":[{"line_number":29,"context_line":"   sense. It doesn\u0027t validate anything and moreover we were skipping"},{"line_number":30,"context_line":"   containers which failed that \"validation\" (!!!)."},{"line_number":31,"context_line":"   In this patch, I commented the variables, suggesting we should change"},{"line_number":32,"context_line":"   them and also converted the \"continue\" into a \"raise\", and increased"},{"line_number":33,"context_line":"   the log level from debug to error if validation didn\u0027t pass."},{"line_number":34,"context_line":"   We should rework that piece."},{"line_number":35,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_9bb3154e","line":32,"updated":"2019-08-09 09:33:04.000000000","message":"Doing that would change the behavior between docker and podman. In case of docker, we would raise and stop iterating over other containers, in case of podman, we would only log the error about not existing directories (in base.py:135) and continue executing other containers.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"381e8cb944b03cec8d2607a5e0e1ecf48e497d5b","unresolved":false,"context_lines":[{"line_number":30,"context_line":"   containers which failed that \"validation\" (!!!)."},{"line_number":31,"context_line":"   In this patch, I commented the variables, suggesting we should change"},{"line_number":32,"context_line":"   them and also converted the \"continue\" into a \"raise\", and increased"},{"line_number":33,"context_line":"   the log level from debug to error if validation didn\u0027t pass."},{"line_number":34,"context_line":"   We should rework that piece."},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"Closes-Bug: #1839559"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_5becbd28","line":33,"updated":"2019-08-09 09:33:04.000000000","message":"There is logging in the validation code with the level of error.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"c2d25fe2c76ab890f5898a970798ed181c5417b1","unresolved":false,"context_lines":[{"line_number":30,"context_line":"   containers which failed that \"validation\" (!!!)."},{"line_number":31,"context_line":"   In this patch, I commented the variables, suggesting we should change"},{"line_number":32,"context_line":"   them and also converted the \"continue\" into a \"raise\", and increased"},{"line_number":33,"context_line":"   the log level from debug to error if validation didn\u0027t pass."},{"line_number":34,"context_line":"   We should rework that piece."},{"line_number":35,"context_line":""},{"line_number":36,"context_line":"Closes-Bug: #1839559"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7faddb67_1b12c50a","line":33,"in_reply_to":"7faddb67_5becbd28","updated":"2019-08-09 09:39:49.000000000","message":"https://github.com/openstack/paunch/blob/master/paunch/builder/compose1.py#L125-L128","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"}],"paunch/builder/base.py":[{"author":{"_account_id":7353,"name":"Kevin Carter","email":"kevin@cloudnull.com","username":"cloudnull"},"change_message_id":"fdbe6c598f186282d478952c5c00fb45243dcca5","unresolved":false,"context_lines":[{"line_number":121,"context_line":"            if not validations_passed:"},{"line_number":122,"context_line":"                self.log.error(\u0027Validations failed for container: %s\u0027 %"},{"line_number":123,"context_line":"                               container)"},{"line_number":124,"context_line":"                raise"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"            (cmd_stdout, cmd_stderr, returncode) \u003d self.runner.execute("},{"line_number":127,"context_line":"                cmd, self.log)"}],"source_content_type":"text/x-python","patch_set":1,"id":"7faddb67_74675153","line":124,"updated":"2019-08-08 22:58:15.000000000","message":"I think you should raise an exception like SystemError(\u0027...\u0027). In that way it\u0027d log the error and return an exception message.","commit_id":"2594c78351031915ccf37a2c237690de250230af"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"6e3d65a48cd7359af026bedcab5b83bea6286542","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                if container in desired_names:"},{"line_number":79,"context_line":"                    discover_container \u003d self.runner.discover_container_name("},{"line_number":80,"context_line":"                        container, self.config_id)"},{"line_number":81,"context_line":"                    inspect_container \u003d self.runner.inspect(discover_container)"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"                    try:"},{"line_number":84,"context_line":"                        self.log.debug(\"Container running state for %s: %s.\" %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_a7a02e93","line":81,"range":{"start_line":81,"start_character":52,"end_line":81,"end_character":59},"updated":"2019-08-09 13:20:21.000000000","message":"we should modify this one to apply the action filters over the running containers only.\n\nThat would save us a 2nd \u0027podman ps\u0027 call, so we could figure out, if exec would work earlier","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"421ceb32a49fd8d2ff06b839d8a51d713685889e","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                if container in desired_names:"},{"line_number":79,"context_line":"                    discover_container \u003d self.runner.discover_container_name("},{"line_number":80,"context_line":"                        container, self.config_id)"},{"line_number":81,"context_line":"                    inspect_container \u003d self.runner.inspect(discover_container)"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"                    try:"},{"line_number":84,"context_line":"                        self.log.debug(\"Container running state for %s: %s.\" %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_f23ee689","line":81,"range":{"start_line":81,"start_character":52,"end_line":81,"end_character":59},"in_reply_to":"7faddb67_12f622fb","updated":"2019-08-09 14:00:32.000000000","message":"...or is it container_names()?..","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"136f5d2e47a0c094a1cb1c09c5acc8323d2f6d0e","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                if container in desired_names:"},{"line_number":79,"context_line":"                    discover_container \u003d self.runner.discover_container_name("},{"line_number":80,"context_line":"                        container, self.config_id)"},{"line_number":81,"context_line":"                    inspect_container \u003d self.runner.inspect(discover_container)"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"                    try:"},{"line_number":84,"context_line":"                        self.log.debug(\"Container running state for %s: %s.\" %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_12f622fb","line":81,"range":{"start_line":81,"start_character":52,"end_line":81,"end_character":59},"in_reply_to":"7faddb67_a7a02e93","updated":"2019-08-09 13:58:00.000000000","message":"I think I misidentified the place where we have the 1st podman ps -a call.\n\nIt is likely current_config_ids() and containers_in_config() calls that need to be extended to support the look among running only flag and custom filters.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"982c184a307ddd36fc888590f94832842c96b78e","unresolved":false,"context_lines":[{"line_number":107,"context_line":"                self.log.debug(\"Start container {}.\".format(container))"},{"line_number":108,"context_line":"                # TODO(emilien) Not sure if validations_passed is really"},{"line_number":109,"context_line":"                # the right name for this variable, as it doesn\u0027t validate"},{"line_number":110,"context_line":"                # much. We probably want to rename it."},{"line_number":111,"context_line":"                validations_passed \u003d self.container_run_args(cmd, container)"},{"line_number":112,"context_line":"            elif action \u003d\u003d \u0027exec\u0027:"},{"line_number":113,"context_line":"                cmd \u003d [self.runner.cont_cmd, \u0027exec\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_18b96be8","line":110,"updated":"2019-08-09 08:54:19.000000000","message":"This variable is only used to indicate if the (currently one) validation passed, nothing more or less. I added doc strings in the *_exec_args and *_run_args methods, so you would also want to change those if you want to change the name or propose a new structure.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":20172,"name":"Michele Baldessari","email":"michele@acksyn.org","username":"michele"},"change_message_id":"99796b4b364883408667ee4b61aa46a3605c9e47","unresolved":false,"context_lines":[{"line_number":116,"context_line":""},{"line_number":117,"context_line":"                if not self.runner.container_running(container):"},{"line_number":118,"context_line":"                    self.log.error(\"Container %s is not running\" % container)"},{"line_number":119,"context_line":"                    raise"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"            if not validations_passed:"},{"line_number":122,"context_line":"                self.log.error(\u0027Validations failed for container: %s\u0027 %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_657479d1","line":119,"range":{"start_line":119,"start_character":16,"end_line":119,"end_character":25},"updated":"2019-08-09 04:26:29.000000000","message":"You need to raise a proper exception here. CI is failing with:\n\"exceptions must be old-style classes or derived from BaseException, not NoneType\n(https://logs.opendev.org/94/675494/2/check/tripleo-ci-centos-7-containers-multinode/40323bc/logs/undercloud/home/zuul/undercloud_install.log.txt.gz#_2019-08-09_00_18_40)","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":3153,"name":"Emilien Macchi","email":"emilien@redhat.com","username":"emilienm"},"change_message_id":"45d767cdfc23454648510db4f843ba4828ad9c69","unresolved":false,"context_lines":[{"line_number":116,"context_line":""},{"line_number":117,"context_line":"                if not self.runner.container_running(container):"},{"line_number":118,"context_line":"                    self.log.error(\"Container %s is not running\" % container)"},{"line_number":119,"context_line":"                    raise"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"            if not validations_passed:"},{"line_number":122,"context_line":"                self.log.error(\u0027Validations failed for container: %s\u0027 %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_a7c90e5b","line":119,"range":{"start_line":119,"start_character":16,"end_line":119,"end_character":25},"in_reply_to":"7faddb67_583123a2","updated":"2019-08-09 13:21:03.000000000","message":"I disagree here, if a container fails for whatever reason, we should not continue with other containers and fail as early as possible.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"982c184a307ddd36fc888590f94832842c96b78e","unresolved":false,"context_lines":[{"line_number":116,"context_line":""},{"line_number":117,"context_line":"                if not self.runner.container_running(container):"},{"line_number":118,"context_line":"                    self.log.error(\"Container %s is not running\" % container)"},{"line_number":119,"context_line":"                    raise"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"            if not validations_passed:"},{"line_number":122,"context_line":"                self.log.error(\u0027Validations failed for container: %s\u0027 %"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_583123a2","line":119,"range":{"start_line":119,"start_character":16,"end_line":119,"end_character":25},"in_reply_to":"7faddb67_657479d1","updated":"2019-08-09 08:54:19.000000000","message":"The raise keyword is used for re-raising an exception after you caught it and you don\u0027t want the stack trace to include this line. I don\u0027t see why it\u0027s used here...\n\nAlso, are you sure you want to throw an exception? This code is iterating on a set of containers and an error in one shouldn\u0027t impact others, that\u0027s why the \"continue\" keyword is originally used.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"ef1391853fd964af7654cad407191afaad2fc18c","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if cmd_stderr:"},{"line_number":132,"context_line":"                stderr.append(cmd_stderr)"},{"line_number":133,"context_line":"            if returncode not in exit_codes:"},{"line_number":134,"context_line":"                self.log.error(\"Error running %s. [%s]\\n\" % (cmd, returncode))"},{"line_number":135,"context_line":"                self.log.error(\"stdout: %s\" % cmd_stdout)"},{"line_number":136,"context_line":"                self.log.error(\"stderr: %s\" % cmd_stderr)"},{"line_number":137,"context_line":"                deploy_status_code \u003d returncode"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_787e5fb0","line":134,"updated":"2019-08-09 09:19:05.000000000","message":"Maybe we don\u0027t want to check if the container is running, but return an exception at this point?","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":3153,"name":"Emilien Macchi","email":"emilien@redhat.com","username":"emilienm"},"change_message_id":"45d767cdfc23454648510db4f843ba4828ad9c69","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            if cmd_stderr:"},{"line_number":132,"context_line":"                stderr.append(cmd_stderr)"},{"line_number":133,"context_line":"            if returncode not in exit_codes:"},{"line_number":134,"context_line":"                self.log.error(\"Error running %s. [%s]\\n\" % (cmd, returncode))"},{"line_number":135,"context_line":"                self.log.error(\"stdout: %s\" % cmd_stdout)"},{"line_number":136,"context_line":"                self.log.error(\"stderr: %s\" % cmd_stderr)"},{"line_number":137,"context_line":"                deploy_status_code \u003d returncode"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_671a3670","line":134,"in_reply_to":"7faddb67_787e5fb0","updated":"2019-08-09 13:21:03.000000000","message":"I don\u0027t see the point of running an exception after the exec (L126). Failing earlier would I think be better in the case of a container not running just before the exec.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"}],"paunch/runner.py":[{"author":{"_account_id":27427,"name":"David Peacock","email":"dpeacock@redhat.com","username":"davidjpeacock"},"change_message_id":"0747ddcd513a1cb487e8e5ae734f4c55a6f25b8b","unresolved":false,"context_lines":[{"line_number":367,"context_line":"        (_, _, returncode) \u003d self.execute(cmd, self.log, quiet)"},{"line_number":368,"context_line":"        return returncode \u003d\u003d 0"},{"line_number":369,"context_line":""},{"line_number":370,"context_line":"    def container_running(self, name, quiet\u003dFalse):"},{"line_number":371,"context_line":"        cmd \u003d ["},{"line_number":372,"context_line":"            self.cont_cmd,"},{"line_number":373,"context_line":"            \u0027ps\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_e7c946ac","line":370,"updated":"2019-08-09 13:06:32.000000000","message":"This block looks correct to me.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"421ceb32a49fd8d2ff06b839d8a51d713685889e","unresolved":false,"context_lines":[{"line_number":367,"context_line":"        (_, _, returncode) \u003d self.execute(cmd, self.log, quiet)"},{"line_number":368,"context_line":"        return returncode \u003d\u003d 0"},{"line_number":369,"context_line":""},{"line_number":370,"context_line":"    def container_running(self, name, quiet\u003dFalse):"},{"line_number":371,"context_line":"        cmd \u003d ["},{"line_number":372,"context_line":"            self.cont_cmd,"},{"line_number":373,"context_line":"            \u0027ps\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_b2486e2e","line":370,"in_reply_to":"7faddb67_52d57a42","updated":"2019-08-09 14:00:32.000000000","message":"(or rather container_names() )","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"},{"author":{"_account_id":6926,"name":"Bogdan Dobrelya","email":"bdobreli@redhat.com","username":"bogdando"},"change_message_id":"136f5d2e47a0c094a1cb1c09c5acc8323d2f6d0e","unresolved":false,"context_lines":[{"line_number":367,"context_line":"        (_, _, returncode) \u003d self.execute(cmd, self.log, quiet)"},{"line_number":368,"context_line":"        return returncode \u003d\u003d 0"},{"line_number":369,"context_line":""},{"line_number":370,"context_line":"    def container_running(self, name, quiet\u003dFalse):"},{"line_number":371,"context_line":"        cmd \u003d ["},{"line_number":372,"context_line":"            self.cont_cmd,"},{"line_number":373,"context_line":"            \u0027ps\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"7faddb67_52d57a42","line":370,"in_reply_to":"7faddb67_e7c946ac","updated":"2019-08-09 13:58:00.000000000","message":"As I noted earlier, let\u0027s move this ps / ps -a + custom --filter values into current_config_ids() and containers_in_config() methods. That saves the 2nd call for the podman ps executed in this redundant place.","commit_id":"8fc93dd537e9007e61eb852a0e585d4a7fa2e491"}]}