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 2020/12/01 14:03:37 UTC
[GitHub] [airflow] potiuk opened a new pull request #12735: User-friendly output of Breeze and CI scripts
potiuk opened a new pull request #12735:
URL: https://github.com/apache/airflow/pull/12735
<!--
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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533489550
##########
File path: breeze
##########
@@ -1361,9 +1361,9 @@ function breeze::parse_arguments() {
;;
*)
breeze::usage
- echo >&2
- echo >&2 "ERROR: Unknown command ${1}"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: Unknown command\e[0m"
+ echo
Review comment:
> Terminals, no, but cases when output is not actually a terminal.
Not a problem for CI/dev tools.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736571804
Example:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533489981
##########
File path: breeze
##########
@@ -2800,12 +2800,12 @@ function breeze::check_and_save_all_params() {
if [[ ${PYTHON_MAJOR_MINOR_VERSION} == "2.7" || ${PYTHON_MAJOR_MINOR_VERSION} == "3.5" ]]; then
if [[ ${BRANCH_NAME} == "master" ]]; then
- echo >&2
- echo >&2 "The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*"
- echo >&2
- echo >&2 "You can use it only when you specify 1.10 Airflow via --install-airflow-version"
- echo >&2 "or --install-airflow-reference and they point to 1.10 version of Airflow"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*\e[0m"
Review comment:
I think something like this would ease readability:
```suggestion
echo " ${COLOR_RED}ERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*${COLOR_RESET}"
```
```sh
COLOR_RED=$'\e[31m'
COLOR_RESET=$'\e[0m'
# etc.
```
(Or we could try associative arrays and then `${color[red]}`)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736609998
> > Looks good but will be pain in terminals that do not support colours
>
> Are there any? :) Those are dev tools. If you do not have color output, you are doomed :)
>
> To be frank - yeah I thought about it but to be honest, benefits outweight the potential downside by a few orders of magnitude IMHO.
Terminals, no, but cases when output is not actually a terminal.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736571804
Example (but it is improved in many, many places):
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533492150
##########
File path: breeze
##########
@@ -2800,12 +2800,12 @@ function breeze::check_and_save_all_params() {
if [[ ${PYTHON_MAJOR_MINOR_VERSION} == "2.7" || ${PYTHON_MAJOR_MINOR_VERSION} == "3.5" ]]; then
if [[ ${BRANCH_NAME} == "master" ]]; then
- echo >&2
- echo >&2 "The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*"
- echo >&2
- echo >&2 "You can use it only when you specify 1.10 Airflow via --install-airflow-version"
- echo >&2 "or --install-airflow-reference and they point to 1.10 version of Airflow"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*\e[0m"
Review comment:
Yeah. Why not :).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736641932
Now it's even nicer :). fixed a few mistakes along the way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533493159
##########
File path: breeze
##########
@@ -2800,12 +2800,12 @@ function breeze::check_and_save_all_params() {
if [[ ${PYTHON_MAJOR_MINOR_VERSION} == "2.7" || ${PYTHON_MAJOR_MINOR_VERSION} == "3.5" ]]; then
if [[ ${BRANCH_NAME} == "master" ]]; then
- echo >&2
- echo >&2 "The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*"
- echo >&2
- echo >&2 "You can use it only when you specify 1.10 Airflow via --install-airflow-version"
- echo >&2 "or --install-airflow-reference and they point to 1.10 version of Airflow"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*\e[0m"
Review comment:
I mean _I_ know that `31m` is red, but don't ask me about any other colours
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736579250
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736581497
> Looks good but will be pain in terminals that do not support colours
Are there any? :) Those are dev tools. If you do not have color output, you are doomed :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736641932
Now it's even nicer :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736644171
Slightly less chatty - and output is now fully "blue" to distinguish it from real errors:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #12735:
URL: https://github.com/apache/airflow/pull/12735
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533481925
##########
File path: breeze
##########
@@ -1361,9 +1361,9 @@ function breeze::parse_arguments() {
;;
*)
breeze::usage
- echo >&2
- echo >&2 "ERROR: Unknown command ${1}"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: Unknown command\e[0m"
+ echo
Review comment:
Why not on stderr anymore?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533494733
##########
File path: breeze
##########
@@ -1361,9 +1361,9 @@ function breeze::parse_arguments() {
;;
*)
breeze::usage
- echo >&2
- echo >&2 "ERROR: Unknown command ${1}"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: Unknown command\e[0m"
+ echo
Review comment:
> Why not on stderr anymore?
The main reason for printing it to stderr is that some tools will colour it differently (red).It used to be standard way when terminals did not support colors, but now when all of them do, it's rather useful for dev tools.
There are no other benefits unless the script/tool is used in Kubernetes, etc. for running in production (I those cases intact) or when the output of script is parsed by another tool (none of those scripts produced parseable outout).
Those scripts are meant to be run in two cases only:
1) When user runs in it manually
2) When they run in CI environment
In both cases coloring part of the output manually is better than redirecting to stderr nowadays. All the modern CI systems and all the terminals for developers (including thsoe embedded in cloud tools and IDEs) support coloured output. Similarly as refresh button in Airflow UI, let's move to the XXI century.
##########
File path: breeze
##########
@@ -1361,9 +1361,9 @@ function breeze::parse_arguments() {
;;
*)
breeze::usage
- echo >&2
- echo >&2 "ERROR: Unknown command ${1}"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: Unknown command\e[0m"
+ echo
Review comment:
> Why not on stderr anymore?
The main reason for printing it to stderr is that some tools will colour it differently (red).It used to be standard way when terminals did not support colors, but now when all of them do, it's rather useful for dev tools.
There are no other benefits unless the script/tool is used in Kubernetes, etc. for running in production (I left those cases intact) or when the output of script is parsed by another tool (none of those scripts produced parseable outout).
Those scripts are meant to be run in two cases only:
1) When user runs in it manually
2) When they run in CI environment
In both cases coloring part of the output manually is better than redirecting to stderr nowadays. All the modern CI systems and all the terminals for developers (including thsoe embedded in cloud tools and IDEs) support coloured output. Similarly as refresh button in Airflow UI, let's move to the XXI century.
##########
File path: breeze
##########
@@ -1361,9 +1361,9 @@ function breeze::parse_arguments() {
;;
*)
breeze::usage
- echo >&2
- echo >&2 "ERROR: Unknown command ${1}"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: Unknown command\e[0m"
+ echo
Review comment:
> Why not on stderr anymore?
The main reason for printing it to stderr is that some tools will colour it differently (red).It used to be standard way when terminals did not support colors, but now when all of them do, it's rather useful for dev tools.
There are no other benefits unless the script/tool is used in Kubernetes, etc. for running in production (I left those cases intact) or when the output of script is parsed by another tool (none of those scripts produced parseable outout).
Those scripts are meant to be run in two cases only:
1) When user runs it manually
2) When they run in CI environment
In both cases coloring part of the output manually is better than redirecting to stderr nowadays. All the modern CI systems and all the terminals for developers (including thsoe embedded in cloud tools and IDEs) support coloured output. Similarly as refresh button in Airflow UI, let's move to the XXI century.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#issuecomment-736581497
> Looks good but will be pain in terminals that do not support colours
Are there any? :) Those are dev tools. If you do not have color output, you are doomed :)
To be frank - yeah I thought about it but to be honest, benefits outweight the potential downside by a few orders of magnitude IMHO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533513981
##########
File path: breeze
##########
@@ -2800,12 +2800,12 @@ function breeze::check_and_save_all_params() {
if [[ ${PYTHON_MAJOR_MINOR_VERSION} == "2.7" || ${PYTHON_MAJOR_MINOR_VERSION} == "3.5" ]]; then
if [[ ${BRANCH_NAME} == "master" ]]; then
- echo >&2
- echo >&2 "The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*"
- echo >&2
- echo >&2 "You can use it only when you specify 1.10 Airflow via --install-airflow-version"
- echo >&2 "or --install-airflow-reference and they point to 1.10 version of Airflow"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*\e[0m"
Review comment:
I prefer not to use arrays if not absolutely needed though - especially on Mac OS /Bash 3 they get tricky. And extracting the common prefixes (ERROR/OK/WARNING) in those vars is even 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #12735: User-friendly output of Breeze and CI scripts
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #12735:
URL: https://github.com/apache/airflow/pull/12735#discussion_r533552954
##########
File path: breeze
##########
@@ -2800,12 +2800,12 @@ function breeze::check_and_save_all_params() {
if [[ ${PYTHON_MAJOR_MINOR_VERSION} == "2.7" || ${PYTHON_MAJOR_MINOR_VERSION} == "3.5" ]]; then
if [[ ${BRANCH_NAME} == "master" ]]; then
- echo >&2
- echo >&2 "The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*"
- echo >&2
- echo >&2 "You can use it only when you specify 1.10 Airflow via --install-airflow-version"
- echo >&2 "or --install-airflow-reference and they point to 1.10 version of Airflow"
- echo >&2
+ echo
+ echo -e " \e[31mERROR: The ${PYTHON_MAJOR_MINOR_VERSION} can only be used when installing Airflow 1.10.*\e[0m"
Review comment:
> I mean _I_ know that `31m` is red, but don't ask me about any other colours
Well. I would not recognize CYAN or MAGENTA either ;)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org