[GitHub] [airflow] potiuk commented on a diff in pull request #23308: Breeze fixups

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

potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860266015


##########
dev/breeze/src/airflow_breeze/utils/confirm.py:
##########
@@ -75,9 +75,8 @@ def user_confirm(
             else:
                 print(f"Wrong answer given {user_status}. Should be one of {allowed_answers}. Try again.")
         except TimeoutOccurred:
-            if default_answer is not None:
-                return default_answer
-            print(f"Timeout after {timeout} seconds. Try again.")
+            print(f"Timeout after {timeout} seconds. Quitting.")

Review Comment:
   This is not good change. It changes the semantics of `default_answer`..
   
   Sometimes we want to quit when there is a timeout, but sometimes we want to continue with "no" or "yes" as default answer - it depends. And this is how it should be IMHO.
   
   This happens for example when image build is proposed or reinstallation is proposed and we should continue with current image/installation ("no") rather than stop ("quit"). This what "default_answer" is for. 



##########
dev/breeze/src/airflow_breeze/utils/confirm.py:
##########
@@ -45,9 +45,9 @@ def user_confirm(
 
     :rtype: object
     :param message: message to display to the user (should end with the question mark)
-    :param timeout: time given user to answer
-    :param default_answer: default value returned on timeout. If no default - the question is
-        repeated until user answers.
+    :param timeout: time (in seconds) given user to answer
+    :param default_answer: default value returned on timeout. If no default answer is
+        given the timeout is disabled.

Review Comment:
   Makes perfect sense. yeah :)



##########
breeze:
##########
@@ -64,7 +64,7 @@ function check_breeze_installed() {
             if "${MY_DIR}/scripts/tools/confirm" "Installing breeze?"; then
                 pipx ensurepath --force
                 pipx install -e "${MY_DIR}/dev/breeze/" --force
-                ${BREEZE_BINARY} setup-autocomplete --force --answer yes
+                ${BREEZE_BINARY} setup-autocomplete

Review Comment:
   I think we should rather warn about replacing the auto-complete in the first question and continue using --force --yes here. Asking question twice only makes sense when you do something really disruptive and want the user to absolutely confirm that they know what they are doing.
   
   Installing breeze without autocomplete makes little sense and you can already do it with:
   
   ```
   pipx install -e ./dev/breeze
   ```
   
   This is described in the docs as "viable option".
   
   But the `./breeze` script is just "convenience installation" and is only here for backwards-compatibility with old breeze and will be eventually renamed to `./scripts/tools/install_breeze.sh` iI think . So it should do a "complete instlalation" - including the autocomplete. 
   
   Why ? Big part of `breeze's` power comes from autocomplete being available (for example using `static-checks` instead of `pre-commit` is justified because we can auto-complete all pre-commit checks. Unfortunately the original author of pre-commit is quite allergic to autocomplete https://github.com/pre-commit/pre-commit/issues/1119  and we had to impleement our own command. So when installing via script we should do both IMHO.
   
   Otherwise the user has to answer yes twice - which makes little sense and is a bit of strange.  Usually when you run 
    a tool or script and you answer "yes" you do not have to repeat the answer. IThere should be IMHO very few cases where users should answer any question twice - mostly when you want to do thing that might be very disruptive.
   
   Happy to hear the reasoning why two questions are better, but I thinl just warning at the first question shoudl be better.



##########
dev/breeze/src/airflow_breeze/commands/configuration_and_maintenance.py:
##########
@@ -234,7 +234,8 @@ def setup_autocomplete(verbose: bool, dry_run: bool, force: bool, answer: Option
     """
     Enables autocompletion of breeze commands.
     """
-    set_forced_answer(answer)
+    if force:

Review Comment:
   This is not consistent with all other commands. All other commmands use just ``--answer`` to provide a default answer (see BREEZE.rst). Force has a different meaning actually. Force is specific to "auto-complete" and it means that it should override the complete in .rc file even if it is there. So those two flags are unrelated



##########
dev/breeze/src/airflow_breeze/commands/configuration_and_maintenance.py:
##########
@@ -247,7 +248,7 @@ def setup_autocomplete(verbose: bool, dry_run: bool, force: bool, answer: Option
     )
     console.print(f"[bright_blue]Activation command script is available here: {autocomplete_path}[/]\n")
     console.print(f"[bright_yellow]We need to add above script to your {detected_shell} profile.[/]\n")
-    given_answer = user_confirm("Should we proceed ?", default_answer=Answer.NO, timeout=3)
+    given_answer = user_confirm("Would you like me to do that automatically?", default_answer=Answer.NO, timeout=30)

Review Comment:
   I thik we shoudl rather remove the timeout here.



##########
dev/breeze/src/airflow_breeze/utils/reinstall.py:
##########
@@ -48,7 +48,7 @@ def ask_to_reinstall_breeze(breeze_sources: Path):
     """
     answer = user_confirm(
         f"Do you want to reinstall Breeze from {breeze_sources.parent.parent}?",
-        timeout=3,
+        timeout=None,

Review Comment:
   This is as expected - that was a deliberate decision: 3 s timeout and NO as default answer are intended here. In this case breeze should not be installed (Default = NO) if the user does not confirm it.
   
   The idea behind it is that Breeze will - in most cases - work fine even if is not reinstalled. User might choose to reinstall it and take the risk (because they are afraid or not time to re-install). And this fine.
   
   The whole idea is with timeout here (and in image build) 
   
   1) to not force the user to do do potentially disruptive operation or an operation that requires internet (reinstalling breeze does).
   2) allow the operation to continue automatically if the user pays no attention to the question)
   3) but give clear indication that installation **might** be needed 
   
   This has some very nice properties (and I am going to write an ADR about it shortly as I see it is missing) - or maybe you could attempt to write an ADR for that ?
   
   - user can continue working without disruptions of the current workflow 
   - there is this 'slightly annoying" 3 s delay if the user does not choose to reinstall (or build image) - it's short enough to let the user work normally but annoying enough to have them eventually pres yes to get rid of it
   - this leads to "eventually consistent" behaviour - everyone sooner or later will upgrade to the latest version - maybe not immediately, but eventually. 
   - also another good property is that the user gets an indication (by question) that if something does not work (for example when a dependency is not installed) that it **might** not work because they did not upgrade. So if things break and they try again they will most likely press "y" to upgrade.



##########
dev/breeze/src/airflow_breeze/utils/confirm.py:
##########
@@ -75,9 +75,8 @@ def user_confirm(
             else:

Review Comment:
   Actually I think there is another bug here. 
   
   Default_answer should have two meanings:
   
   1) when timeout is ther and you do not answer, the default one is returned.
   2) when you press enter without providing answer and default is set - the default answer should be returned.
   
   Case 2) is not handled here only 1) actually. It would be great to fix it while you are at it.



-- 
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