[GitHub] [airflow] ferruzzi commented on a diff in pull request #27191: Make dry_run, verbose, answer breeze flags static

[GitBox <gi...@apache.org>]

ferruzzi commented on code in PR #27191:
URL: https://github.com/apache/airflow/pull/27191#discussion_r1005158642


##########
dev/breeze/src/airflow_breeze/utils/shared_options.py:
##########
@@ -0,0 +1,61 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import os
+
+__verbose_value: bool = os.environ.get("VERBOSE", "false")[0].lower() == "t"
+
+
+def set_verbose(verbose: bool):
+    global __verbose_value
+    __verbose_value = verbose
+
+
+def get_verbose(verbose_override: bool | None = None) -> bool:
+    if verbose_override is None:
+        return __verbose_value
+    return verbose_override

Review Comment:
   Nitpick:  Here and below, these getters could all just be oneliners: `return verbose_override or __verbose_value` if you want to trim them down.



##########
dev/breeze/src/airflow_breeze/commands/testing_commands.py:
##########
@@ -84,20 +84,18 @@ def testing():
         allow_extra_args=True,
     ),
 )
-@option_verbose
-@option_dry_run
 @option_python
-@option_github_repository
 @option_image_tag_for_running
 @option_image_name
+@option_github_repository
+@option_verbose
+@option_dry_run
 @click.argument("extra_pytest_args", nargs=-1, type=click.UNPROCESSED)
 def docker_compose_tests(
-    verbose: bool,
-    dry_run: bool,
     python: str,
-    github_repository: str,
     image_name: str,
     image_tag: str | None,
+    github_repository: str,

Review Comment:
   I'm not too familiar with click yet, will this magically use the provider or do you need to default it with get_github_repository()? 



##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -197,44 +187,38 @@ def prepare_airflow_packages(
     name="prepare-provider-documentation",
     help="Prepare CHANGELOG, README and COMMITS information for providers.",
 )
-@option_verbose
-@option_dry_run
+@option_debug_release_management
+@argument_packages
 @click.option(
     "--base-branch",
     type=str,
     default="main",
 )
 @option_github_repository
+@option_verbose
+@option_dry_run
 @option_answer
-@option_debug_release_management
-@argument_packages
 def prepare_provider_documentation(
-    verbose: bool,
-    dry_run: bool,
-    base_branch: str,
     github_repository: str,
-    answer: str | None,
+    base_branch: str,
     debug: bool,
     packages: list[str],
 ):
-    perform_environment_checks(verbose=verbose)
+    perform_environment_checks()
     shell_params = ShellParams(
-        verbose=verbose,
         mount_sources=MOUNT_ALL,
         github_repository=github_repository,
         python=DEFAULT_PYTHON_MAJOR_MINOR_VERSION,
-        answer=answer,
         base_branch=base_branch,
         skip_environment_initialization=True,
     )
-    rebuild_or_pull_ci_image_if_needed(command_params=shell_params, dry_run=dry_run, verbose=verbose)
+    rebuild_or_pull_ci_image_if_needed(command_params=shell_params)
     cmd_to_run = ["/opt/airflow/scripts/in_container/run_prepare_provider_documentation.sh", *packages]
+    answer = get_forced_answer()
     result_command = run_with_debug(
         params=shell_params,
         command=cmd_to_run,
-        enable_input=not answer or answer.lower() not in ["y", "yes"],
-        verbose=verbose,
-        dry_run=dry_run,
+        enable_input=answer is None or answer.lower() not in ["y", "yes"],

Review Comment:
   Consider the same formula you sued elsewhere?
   
   ```suggestion
           enable_input=answer is None or answer[0].lower() not in "y",
   ```



##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -546,31 +522,17 @@ def enter_shell(**kwargs) -> RunCommandResult:
     * executes the command to drop the user to Breeze shell
 
     """
-    verbose = kwargs["verbose"]
-    dry_run = kwargs["dry_run"]
-    perform_environment_checks(verbose=verbose)
+    perform_environment_checks()
     if read_from_cache_file("suppress_asciiart") is None:
         get_console().print(ASCIIART, style=ASCIIART_STYLE)
     if read_from_cache_file("suppress_cheatsheet") is None:
         get_console().print(CHEATSHEET, style=CHEATSHEET_STYLE)
-    enter_shell_params = ShellParams(**filter_out_none(**kwargs))
-    rebuild_or_pull_ci_image_if_needed(command_params=enter_shell_params, dry_run=dry_run, verbose=verbose)
-    if enter_shell_params.include_mypy_volume:
+    shell_params = ShellParams(**filter_out_none(**kwargs))
+    rebuild_or_pull_ci_image_if_needed(
+        command_params=shell_params,
+    )

Review Comment:
   may as well drop the comma here so this will collapse down to a one-liner
   
   ```suggestion
       rebuild_or_pull_ci_image_if_needed(command_params=shell_params)
   ```



##########
dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:
##########
@@ -1574,11 +1435,12 @@ def _run_complete_tests(
         output=output,
         num_tries=num_tries,
         force_recreate_cluster=force_recreate_cluster,
-        verbose=verbose,
-        dry_run=dry_run,
     )
     if returncode != 0:
-        _logs(python=python, kubernetes_version=kubernetes_version, dry_run=dry_run, verbose=verbose)
+        _logs(
+            python=python,
+            kubernetes_version=kubernetes_version,
+        )

Review Comment:
   Here and several times below, I'd drop the trailing comma so this collapses down.
   
   ```suggestion
           _logs(python=python, kubernetes_version=kubernetes_version)
   ```



##########
dev/breeze/src/airflow_breeze/commands/developer_commands.py:
##########
@@ -546,31 +522,17 @@ def enter_shell(**kwargs) -> RunCommandResult:
     * executes the command to drop the user to Breeze shell
 
     """
-    verbose = kwargs["verbose"]
-    dry_run = kwargs["dry_run"]
-    perform_environment_checks(verbose=verbose)
+    perform_environment_checks()
     if read_from_cache_file("suppress_asciiart") is None:
         get_console().print(ASCIIART, style=ASCIIART_STYLE)
     if read_from_cache_file("suppress_cheatsheet") is None:
         get_console().print(CHEATSHEET, style=CHEATSHEET_STYLE)
-    enter_shell_params = ShellParams(**filter_out_none(**kwargs))
-    rebuild_or_pull_ci_image_if_needed(command_params=enter_shell_params, dry_run=dry_run, verbose=verbose)
-    if enter_shell_params.include_mypy_volume:
+    shell_params = ShellParams(**filter_out_none(**kwargs))
+    rebuild_or_pull_ci_image_if_needed(
+        command_params=shell_params,
+    )
+    if shell_params.include_mypy_volume:
         create_mypy_volume_if_needed()
-    return run_shell(verbose, dry_run, enter_shell_params)
-
-
-def run_shell(verbose: bool, dry_run: bool, shell_params: ShellParams) -> RunCommandResult:
-    """
-    Executes a shell command built from params passed.
-    * prints information about the build
-    * constructs docker compose command to enter shell
-    * executes it
-
-    :param verbose: print commands when running
-    :param dry_run: do not execute "write" commands - just print what would happen
-    :param shell_params: parameters of the execution
-    """

Review Comment:
   Nice, assuming this is the only place that method was used, this makes a lot more sense.



##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -612,15 +565,17 @@ def release_prod_images(
                 "[info]Also tagging the images with latest tags as this is release version.[/]"
             )
     result_docker_buildx = run_command(
-        ["docker", "buildx", "version"], check=False, dry_run=dry_run, verbose=verbose
+        ["docker", "buildx", "version"],
+        check=False,

Review Comment:
   Trailing comma here and on L578, but not on L588.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org