From 6719a53c81aeed8f968642e85c812bb55ec8f2b7 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Wed, 21 Dec 2022 02:34:11 +0100 Subject: [PATCH] Fix selective checks handling error tracebacks in CI Initially selective check was implemented in the way that it printed diagnostic output on stdout and the GITHUB_OUTPUT compatible set of outputs on stderr so that it could be redirected to the GITHUB_OUTPUT in its entirety. But this turned out to be a bad idea because when there was an error generated in selective-checks themselves, the traceback was printed in stderr and redirecting stderr to GITHUB_OUTPUT swallowed the traceback. This change reverses the behaviour: * diagnostic output is printed to stderr * GITHUB_OUTPUT compatible output is printed to stdout This way when traceback happens it is printed to stderr and is not swalleowed by redirection to GITHUB_OUTPUT --- .github/workflows/ci.yml | 3 +- .../airflow_breeze/commands/ci_commands.py | 12 +++---- .../airflow_breeze/utils/github_actions.py | 4 +-- .../airflow_breeze/utils/selective_checks.py | 36 +++++++++---------- 4 files changed, 28 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7cf54ccb7b2b4d..83d55acfea8211 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -221,7 +221,8 @@ jobs: env: PR_LABELS: "${{ steps.source-run-info.outputs.pr-labels }}" COMMIT_REF: "${{ github.sha }}" - run: breeze ci selective-check 2>> ${GITHUB_OUTPUT} + VERBOSE: "false" + run: breeze ci selective-check >> ${GITHUB_OUTPUT} - name: env run: printenv env: diff --git a/dev/breeze/src/airflow_breeze/commands/ci_commands.py b/dev/breeze/src/airflow_breeze/commands/ci_commands.py index 39de15a76a08aa..e62875630be7f9 100644 --- a/dev/breeze/src/airflow_breeze/commands/ci_commands.py +++ b/dev/breeze/src/airflow_breeze/commands/ci_commands.py @@ -50,7 +50,7 @@ option_verbose, ) from airflow_breeze.utils.confirm import Answer, user_confirm -from airflow_breeze.utils.console import get_console +from airflow_breeze.utils.console import get_console, get_stderr_console from airflow_breeze.utils.custom_param_types import BetterChoice from airflow_breeze.utils.docker_command_utils import ( check_docker_resources, @@ -179,14 +179,14 @@ def get_changed_files(commit_ref: str | None) -> tuple[str, ...]: ] result = run_command(cmd, check=False, capture_output=True, text=True) if result.returncode != 0: - get_console().print( + get_stderr_console().print( f"[warning] Error when running diff-tree command [/]\n{result.stdout}\n{result.stderr}" ) return () changed_files = tuple(result.stdout.splitlines()) if result.stdout else () - get_console().print("\n[info]Changed files:[/]\n") - get_console().print(changed_files) - get_console().print() + get_stderr_console().print("\n[info]Changed files:[/]\n") + get_stderr_console().print(changed_files) + get_stderr_console().print() return changed_files @@ -250,7 +250,7 @@ def selective_check( pr_labels=tuple(ast.literal_eval(pr_labels)) if pr_labels else (), github_event=github_event, ) - print(str(sc), file=sys.stderr) + print(str(sc), file=sys.stdout) @ci_group.command(name="find-newer-dependencies", help="Finds which dependencies are being upgraded.") diff --git a/dev/breeze/src/airflow_breeze/utils/github_actions.py b/dev/breeze/src/airflow_breeze/utils/github_actions.py index 6b8043aa7e7269..1566bbe81c8d6d 100644 --- a/dev/breeze/src/airflow_breeze/utils/github_actions.py +++ b/dev/breeze/src/airflow_breeze/utils/github_actions.py @@ -20,11 +20,11 @@ from rich.markup import escape -from airflow_breeze.utils.console import get_console +from airflow_breeze.utils.console import get_stderr_console def get_ga_output(name: str, value: Any) -> str: output_name = name.replace("_", "-") printed_value = str(value).lower() if isinstance(value, bool) else value - get_console().print(f"[info]{output_name}[/] = [green]{escape(str(printed_value))}[/]") + get_stderr_console().print(f"[info]{output_name}[/] = [green]{escape(str(printed_value))}[/]") return f"{output_name}={printed_value}" diff --git a/dev/breeze/src/airflow_breeze/utils/selective_checks.py b/dev/breeze/src/airflow_breeze/utils/selective_checks.py index eeaf65150ea60e..d978434b3c083f 100644 --- a/dev/breeze/src/airflow_breeze/utils/selective_checks.py +++ b/dev/breeze/src/airflow_breeze/utils/selective_checks.py @@ -54,7 +54,7 @@ SelectiveUnitTestTypes, all_selective_test_types, ) -from airflow_breeze.utils.console import get_console +from airflow_breeze.utils.console import get_stderr_console FULL_TESTS_NEEDED_LABEL = "full tests needed" DEBUG_CI_RESOURCES_LABEL = "debug ci resources" @@ -295,16 +295,16 @@ def default_constraints_branch(self) -> str: @cached_property def full_tests_needed(self) -> bool: if not self._commit_ref: - get_console().print("[warning]Running everything as commit is missing[/]") + get_stderr_console().print("[warning]Running everything as commit is missing[/]") return True if self._github_event in [GithubEvents.PUSH, GithubEvents.SCHEDULE, GithubEvents.WORKFLOW_DISPATCH]: - get_console().print(f"[warning]Full tests needed because event is {self._github_event}[/]") + get_stderr_console().print(f"[warning]Full tests needed because event is {self._github_event}[/]") return True if len(self._matching_files(FileGroupForCi.ENVIRONMENT_FILES, CI_FILE_GROUP_MATCHES)) > 0: - get_console().print("[warning]Running everything because env files changed[/]") + get_stderr_console().print("[warning]Running everything because env files changed[/]") return True if FULL_TESTS_NEEDED_LABEL in self._pr_labels: - get_console().print( + get_stderr_console().print( "[warning]Full tests needed because " f"label '{FULL_TESTS_NEEDED_LABEL}' is in {self._pr_labels}[/]" ) @@ -434,24 +434,24 @@ def _matching_files(self, match_group: T, match_dict: dict[T, list[str]]) -> lis self._match_files_with_regexps(matched_files, regexps) count = len(matched_files) if count > 0: - get_console().print(f"[warning]{match_group} matched {count} files.[/]") - get_console().print(matched_files) + get_stderr_console().print(f"[warning]{match_group} matched {count} files.[/]") + get_stderr_console().print(matched_files) else: - get_console().print(f"[warning]{match_group} did not match any file.[/]") + get_stderr_console().print(f"[warning]{match_group} did not match any file.[/]") return matched_files def _should_be_run(self, source_area: FileGroupForCi) -> bool: if self.full_tests_needed: - get_console().print(f"[warning]{source_area} enabled because we are running everything[/]") + get_stderr_console().print(f"[warning]{source_area} enabled because we are running everything[/]") return True matched_files = self._matching_files(source_area, CI_FILE_GROUP_MATCHES) if len(matched_files) > 0: - get_console().print( + get_stderr_console().print( f"[warning]{source_area} enabled because it matched {len(matched_files)} changed files[/]" ) return True else: - get_console().print( + get_stderr_console().print( f"[warning]{source_area} disabled because it did not match any changed files[/]" ) return False @@ -503,7 +503,7 @@ def _select_test_type_if_matching( count = len(matched_files) if count > 0: test_types.add(test_type.value) - get_console().print(f"[warning]{test_type} added because it matched {count} files[/]") + get_stderr_console().print(f"[warning]{test_type} added because it matched {count} files[/]") return matched_files def _get_test_types_to_run(self) -> list[str]: @@ -528,11 +528,11 @@ def _get_test_types_to_run(self) -> list[str]: remaining_files = set(all_source_files) - set(matched_files) - set(kubernetes_files) count_remaining_files = len(remaining_files) if count_remaining_files > 0: - get_console().print( + get_stderr_console().print( f"[warning]We should run all tests. There are {count_remaining_files} changed " "files that seems to fall into Core/Other category[/]" ) - get_console().print(remaining_files) + get_stderr_console().print(remaining_files) candidate_test_types.update(all_selective_test_types()) else: if "Providers" in candidate_test_types: @@ -540,12 +540,12 @@ def _get_test_types_to_run(self) -> list[str]: if len(affected_providers) != 0: candidate_test_types.remove("Providers") candidate_test_types.add(f"Providers[{','.join(sorted(affected_providers))}]") - get_console().print( + get_stderr_console().print( "[warning]There are no core/other files. Only tests relevant to the changed files are run.[/]" ) sorted_candidate_test_types = list(sorted(candidate_test_types)) - get_console().print("[warning]Selected test type candidates to run:[/]") - get_console().print(sorted_candidate_test_types) + get_stderr_console().print("[warning]Selected test type candidates to run:[/]") + get_stderr_console().print(sorted_candidate_test_types) return sorted_candidate_test_types @cached_property @@ -560,7 +560,7 @@ def test_types(self) -> str: test_types_to_remove: set[str] = set() for test_type in current_test_types: if test_type.startswith("Providers"): - get_console().print( + get_stderr_console().print( f"[warning]Removing {test_type} because the target branch " f"is {self._default_branch} and not main[/]" )