Skip to content

Conversation

@edithturn
Copy link
Contributor

@edithturn edithturn commented Apr 13, 2022

Closes #22908

Steps:

  • First: Reproduced the error, modifying BREEZE.rst, It will update the output of breeze commands
  • In pre_commit_breeze_cmd_line.py added verify_breeze_installed to verify if the breeze is installed. If not print a message.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@edithturn
Copy link
Contributor Author

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

@edithturn edithturn changed the title added intentional change on BREEZE.rst to reproduce the error Add nicer error for breeze update static check Apr 14, 2022
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use shutil.which()? Not as accurate (fails if breeze is not “our” command but from somewhere else, for example), but a lot faster.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Shutil.which is better. It's also portable and it's enough (we don't verify the output of version command anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should use rich console and [red] for error and [yellow] for "action" to be done by the user

@potiuk
Copy link
Member

potiuk commented Apr 14, 2022

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

@potiuk
Copy link
Member

potiuk commented Apr 14, 2022

Added issue for uninstallation instruction: #23005

@edithturn
Copy link
Contributor Author

edithturn commented Apr 14, 2022

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 breeze installation from the static check step and make a change in BREEZE.rst?

@potiuk
Copy link
Member

potiuk commented Apr 14, 2022

  • Should I remove breeze installation from the static check step and make a change in BREEZE.rst?

Yep

@edithturn
Copy link
Contributor Author

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

@edithturn
Copy link
Contributor Author

I think now that I know how to uninstall Breeze I can try this 🕺🏼

@edithturn edithturn force-pushed the add-nicer-error-for-breeze branch 4 times, most recently from d0cc8b5 to 2b7bc93 Compare April 18, 2022 13:59
@edithturn edithturn force-pushed the add-nicer-error-for-breeze branch from 2b7bc93 to 3762f24 Compare April 20, 2022 14:30
@potiuk
Copy link
Member

potiuk commented Apr 25, 2022

I think this one should be closed @edithturn ?

@edithturn
Copy link
Contributor Author

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

@edithturn
Copy link
Contributor Author

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

@edithturn edithturn closed this Apr 27, 2022
@potiuk
Copy link
Member

potiuk commented Apr 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Breeze: Add nicer error for BREEZE.rst update static check

3 participants