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/13 21:19:07 UTC
[GitHub] [airflow] edithturn opened a new pull request, #22998: added intentional change on BREEZE.rst to reproduce the error
edithturn opened a new pull request, #22998:
URL: https://github.com/apache/airflow/pull/22998
Closes #22908
Steps:
- First: Reproduced the error, modifying BREEZE.rst, It will update the output of breeze commands
<!--
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/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 [UPDATING.md](https://github.com/apache/airflow/blob/main/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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #22998:
URL: https://github.com/apache/airflow/pull/22998#discussion_r850156929
##########
scripts/ci/pre_commit/pre_commit_breeze_cmd_line.py:
##########
@@ -79,6 +80,19 @@ def verify_all_commands_described_in_docs():
sys.exit(1)
+def verify_breeze_installed():
+ result = subprocess.getoutput('breeze version')
Review Comment:
Maybe use `shutil.which()`? Not as accurate (fails if `breeze` is not “our” command but from somewhere else, for example), but a lot faster.
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1108905075
I think this one should be closed @edithturn ?
--
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] edithturn commented on pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1109051830
@potiuk I will test in another branch, bcz in this branch I am not able to reproduce the error. But I saw that in a new branch when Breeze is not installed the not friendly message is shown it.
Let me test this in another branch of this same code after doing a rebase.
--
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] edithturn commented on pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1099783759
@potiuk I wasn't able to replicated after the changed I added. https://github.com/apache/airflow/runs/6033018954?check_suite_focus=true
Any other parameter I should take into consideration to replicate it, what I did was:
* Remove the installation of breeze from static check steps
* Add an intentional error in a breeze.rst file
Also that images that are generated each time I make a change in breeze or dev, are not being generating in localmachine but they are in ci server.
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1099348077
> * Should I remove breeze installation from the static check step and make a change in BREEZE.rst?
Yep
--
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] edithturn commented on pull request #22998: added intentional change on BREEZE.rst to reproduce the error
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1098714582
@potiuk, in `pre_commit_breeze_cmd_line.py` I am validating when breeze is not installed.
Some questions I have:
* Breeze in CI should always be installed in the environment, why we should validate the installation for CI?
* One thing I notices is that we don't have instructions to "uninstall breeze" in case the developer wanted to do it.
Please let me know if this is the way to do it or if I have misunderstood this problem.
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1098892447
Added issue for uninstallation instruction: https://github.com/apache/airflow/issues/23005
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1098889991
> * Breeze in CI should always be installed in the environment, why we should validate the installation for CI?
Yeah. It **should**. But it already turned out that it's not always is. See the conversation here: https://github.com/apache/airflow/pull/22961 . We really want people who are not familiar with breeze and CI got precise information about what is wrong. They (even @kaxil !) cannot easily figure out what's wrong when they see a cryptic stack-trace - mostly because they do not have the context. The CI is a living system that changes continuously and we can add new jobs in the future or mistakenly remove stuff. Having a nice message explaining what's wrong AND telling the user what to do is way better than cryptic stack trace.
> * One thing I notices is that we don't have instructions to "uninstall breeze" in case the developer wanted to do it.
Very, very food point - can you add it please (separate PR) :) ? It could be added in the cheatsheeet as well as in the BREEZE.rst.
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22998:
URL: https://github.com/apache/airflow/pull/22998#discussion_r850232461
##########
scripts/ci/pre_commit/pre_commit_breeze_cmd_line.py:
##########
@@ -79,6 +80,19 @@ def verify_all_commands_described_in_docs():
sys.exit(1)
+def verify_breeze_installed():
+ result = subprocess.getoutput('breeze version')
Review Comment:
Yeah. Shutil.which is better. It's also portable and it's enough (we don't verify the output of `version` command anyway.
--
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] edithturn commented on pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1100256621
I think now that I know how to uninstall Breeze I can try this 🕺🏼
--
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] edithturn closed pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn closed pull request #22998: Add nicer error for breeze update static check
URL: https://github.com/apache/airflow/pull/22998
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1110736325
No worries. I will take it on - we had some problem with update-breeze-file running in "CI" giving different results so I will take a closer look there.
--
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] edithturn commented on pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1110028302
@potiuk I will close this PR, I am not able to reproduce it on the local machine, and also it has conflicts that I wasn't able to solve. I created another branch with rebase. Could you guide me on what exactly I should change in Breeze.rst to make the images generate?
--
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 #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #22998:
URL: https://github.com/apache/airflow/pull/22998#discussion_r850233226
##########
scripts/ci/pre_commit/pre_commit_breeze_cmd_line.py:
##########
@@ -79,6 +80,19 @@ def verify_all_commands_described_in_docs():
sys.exit(1)
+def verify_breeze_installed():
+ result = subprocess.getoutput('breeze version')
+ if "Breeze installed from" in result:
+ print_help_for_all_commands()
+ verify_all_commands_described_in_docs()
+ else:
+ print(
+ "You don't have Breeze installed, follow BREEZE.rst to install it \
Review Comment:
we should use rich console and [red] for error and [yellow] for "action" to be done by the user
--
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] edithturn commented on pull request #22998: Add nicer error for breeze update static check
Posted by GitBox <gi...@apache.org>.
edithturn commented on PR #22998:
URL: https://github.com/apache/airflow/pull/22998#issuecomment-1099264986
Thank you for the feedback @potiuk @uranusjr, make sense in adding "precise information about what is wrong".
Now, how I could test this when `breeze` is not installed in ci, it is already in ci and we can't see the behavior.
* Should I remove the installation from the static check and make a change in BREEZE.rst?
--
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