potiuk commented on code in PR #27829: URL: https://github.com/apache/airflow/pull/27829#discussion_r1029227947
########## dev/breeze/src/airflow_breeze/commands/minor_release_command.py: ########## @@ -0,0 +1,165 @@ +# 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. + +import os + +import click + +from airflow_breeze.utils.path_utils import AIRFLOW_SOURCES_ROOT +from airflow_breeze.utils.run_utils import run_command + + +def create_branch(version_branch): + if click.confirm(f"Create version branch: {version_branch}?"): Review Comment: I think it will be better to plugin into the mechanisms we've implemented in Breeze for that `airflow_breeze.utils.confirm` and `airflow_breeze.utils.console`. There are few reasons for it (and why I deliberately chose not to use them) # Confirm So far I always made sure that any command I run manually is also executed automatically in our CI process - this is the only way to keep it "working" especially if those are things that are rarely used (release process). The saying is "if there is something painful", run it more often. My approach for a new command similar to what you are adding here is to add a CI job to actually run the command to some extent at least (i.e. not to make actual release) in CI - just to see that the command continues to work. This is also helpful in case of any future refactorings - if the command will continue to run and get a "green" PR mark, then any refactorings you might want to make is "safer". For that purpose - our `confirm` has a built-in mechanism that rather than asking for input it can base it on `--answer` flag. You can add `--answer yes` and then any confirm operation will act as if user answered `yes`. This is much better solution than the click.confirm one where you would have to literally redirect input from a file or something to get similar result (this is main reason why we have our own util for that. There are few more reasons why we have our own confirm (mostly about timeout) but all the reasons and architecture decision record documenting why and how are nicely documented in this ADR: https://github.com/apache/airflow/blob/main/dev/breeze/doc/adr/0012-asking-user-for-confirmation.md # Echo Regarding the echo, we also deliberately use `rich` console (in our `console` util), to be able to have unified communication mechanism and use colors to communicate with the user (and we even have a mechanism to change breeze configuration to convert it into color-blind-friendly communication). So whenever we communicate something to a user we have to choose what type of the message it is - info, warning, error and use the right rich style. Also the console works well in our CI with appropriate lenght of the screen etc. so it is better to use it for the CI reason. More information about this unified communication style is in this ADR explaining what are the kinds of messages you should pass to the user and how the colour-blind-friendly approach is done: https://github.com/apache/airflow/blob/main/dev/breeze/doc/adr/0011-unified-communication-with-the-users.md -- 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-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
