You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/04/27 21:18:26 UTC

[GitHub] [airflow] blag opened a new pull request, #23308: Breeze fixups

blag opened a new pull request, #23308:
URL: https://github.com/apache/airflow/pull/23308

   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   This PR improves Breeze timeout behavior and tries to ensure that Breeze asks for permission before modifying user config files, which are global to the user.
   
   * The timeout period while waiting for user input to modify their user config file has been increased, giving slow readers more time to read and think through the implications of their choices.
   * The timeout for reinstalling breeze has been disabled. Since we can't know if the user _needs_ to do so or not, we should refuse the temptation to guess either way.
   * Timeouts _without_ default answer values are now ignored, instead of constantly re-prompting the user.
   * Breeze should now explicitly ask for permission before overwriting user config files.
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
   


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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860273943


##########
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 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 questions 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 think just warning at the first question shoudl be better.



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860273943


##########
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 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 questions 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 think just warning the user at the first question should be better.



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860291672


##########
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 should rather remove the timeout here. It is potentially disruptive operation indeed so any timeout here is bad and the user should take some action once they started the action via running command. 
   
   Timeout is mostly reserved for commands which are "by-products".  For example when you run "breeze shell" and image is outdated, you are asked whether to rebuild it - but this is not what you intended to do - you intended to run "breeze shell" command. Asking for image is "accidental" and timeout is there to allow to eventually do the original command.
   
   Here you really want to run the command. This is the intention. So while asking question for confirmation is fine (as this is disruptive command potentially), timeout here makes little sense- because there is nothing else to do on timeout.



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


[GitHub] [airflow] blag closed pull request #23308: Breeze fixups

Posted by GitBox <gi...@apache.org>.
blag closed pull request #23308: Breeze fixups
URL: https://github.com/apache/airflow/pull/23308


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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860273943


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



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860292708


##########
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 that needs fixing. 
   
   Default_answer should have two meanings:
   
   1) when timeout is there and you do not answer, the default one is returned.
   2) regardless from timeout when user presses 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


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

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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860297010


##########
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:
   BTW. originally the `--answer` command was named "--force-answer-to-questions` but I thought it's too long and I shortened it to just `--answer`. Maybe that was too much ? 



##########
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:
   BTW. originally the `--answer` command was named `--force-answer-to-questions` but I thought it's too long and I shortened it to just `--answer`. Maybe that was too much ? 



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860275019


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



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23308:
URL: https://github.com/apache/airflow/pull/23308#issuecomment-1111952093

   Why closing :) ? There were a few things cool and some discussions to complete.


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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860273943


##########
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 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 questions 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.



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


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

Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #23308:
URL: https://github.com/apache/airflow/pull/23308#discussion_r860292708


##########
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 that needs fixing. 
   
   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