Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Apr 24, 2022

The new breeze.py commands grew into separated groups of commands and
breeze.py became far too big.

This change is a refactor to split breeze.py into separated groups of
commands.

No new functionality is added, just refactoring to separate files.

Fixes: #23204


^ 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 a newsfragement file, named {pr_number}.significant.rst, in newsfragments.

The cached parameters in Python Breeze were largely based on
Bash implementation. They did the job but required pretty
cumbersome synchronization of cached values with parameters
passed and it was easy to forget about this as you had to
do it sepearately in each method that had potentially
cacheable parameters.

In this PR we take advantage of the Click class hiarchy and their
extendability. We've already extended Click Choice parameter
to be much better formatted for long list of choices but we take
this a bit further now with adding new type of parameter that
can cache the values between runs.

This has multiple advantages:

* we do not have to remember about synchroniation - parameters
  automatically read/write their values from cache as they are
  used
* we can automatically set parameters to default when wrong
  value is passed. This is nice as user does not have to
  re-run the command because the values are corrected
  on-the-flight.
* the parameters can have their default values displayed in
  help screen (we use sentinel default that we can use to
  detect if value was passed from parameter or taken from default)
* the parameters can also have their <current> values marked in
  list of choices - so the user in the help screen can see not
  only the default but also which value is currently selected
  and will be used if you do not pass any parameter.
@potiuk potiuk changed the title Split breeze commands Split breeze commands across multiple files Apr 24, 2022
@potiuk potiuk requested a review from eladkal April 24, 2022 23:42
@potiuk
Copy link
Member Author

potiuk commented Apr 24, 2022

cc: @Bowrna @eladkal @edithturn - we came to the point when we should split the commands into separate packages

(This change is based on #23195 so it should be merged first).

@potiuk potiuk requested review from malthe and uranusjr April 24, 2022 23:44
The new breeze.py commands grew into separated groups of commands and
breeze.py became far too big.

This change is a refactor to split breeze.py into separated groups of
commands.

No new functionality is added, just refactoring to separate files.

Fixes: apache#23204
@potiuk potiuk force-pushed the split-breeze-commands branch from 2768b83 to 7576632 Compare April 24, 2022 23:47
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Apr 25, 2022
@github-actions
Copy link

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member Author

potiuk commented Apr 25, 2022

I will need to merge #23195 first @eladkal

@potiuk potiuk merged commit e9f9d33 into apache:main Apr 25, 2022
@potiuk potiuk deleted the split-breeze-commands branch April 25, 2022 07:05
@potiuk
Copy link
Member Author

potiuk commented Apr 25, 2022

Rebased and merged both :)

@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split commands in Breeze into multiple files

3 participants