Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Aug 1, 2022

Originally Python version of Breeze had only a handful of commands,
while moving from Bash, but we are close to completion of the
transition and the number of commands grew quite a lot bringit it
on par to the commands we used to have in the Bash version of Breeze.

However, only a small subset of the commands is actually useful to
average developer, most of the more advanced commands are used in
specific circumstances (release management, configuring Breeze
once or rebuilding the image or even runing non-interactive test
session are rarely used. Therefore it makes much more sense to
surface the common commands as the top-level commands and move
the less frequent commands to subcommands to move them out from
the main help page.

At the same time, the BREEZE.rst documentation got a little messy
during the move and this is the right time to structure it similarly
to breeze commands:

  • prerequisites and installation
  • first-time configuration
  • regular task description
  • more advanced tasks grupped in the same subcommands as in Breeze
  • diving deeper into details of Breeze implentation for those
    who wish to understand how Breeze works under-the-hood

Aliases for the common commands that users could already get used to
were created, and deprecation warnings are printed in casee those
commands are used (guiding the user to the new commands to use).

Less frequently used options for shell command are still available
in shell but they have been removed from the default command to
remove clutter. You can still used them by explicitly specifying
the shell commnd.

Configuration for rich click has been separated out from the
command group implementation to separate packages. This allows
for less problems with circular imports - none of the commands
are needed when rich-click configuration is being prepared, which
happens before main click command is parsed, which allows to
run imports of the "code" as late as possible.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

cc: @rossturk @blag - you might be interested in commenting/reviewing.

@potiuk potiuk requested a review from eladkal August 1, 2022 15:17
@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

cc: @edithturn @Bowrna - yet another step towards even better Breeze experience.

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

This one looks big - but most of it is reshuffling the code around to account for moving the Breeze commands to subcommands. A lot of those changes are auto-generated screenshots. We already had the groups before, but the commands were cluttering a lot the initial help screen and they were not logically connected except the rich-click configuration.

Now they are truly separated - including no back-reference from the sub-command to main which was there before. Thanks to that a lot of potentially circular references are avoided.

I used the opportunity to also refactor Breeze docs to follow the most frequently used -> least frequently used pattern and align the groups with the docs, as the documentation was quite messy.

Now it is all much cleaner, neater, logical, smaller and isolated.

I kept invisible aliases to the commands that people could get used to.

New main breeze help is much more readable now:

image

And documentation flow is much more logical:

image

image

@rossturk
Copy link
Contributor

rossturk commented Aug 1, 2022

@potiuk I have successfully run through the start-airflow, prod-image build-image, and compile-www-assets commands. They all seem to work as before. And I like the new subcommand structure 👍

I have noticed that the output no longer wraps properly in terminals less than 180 characters wide. I'm not much of an expert in click, so I'm not sure how to fix this. I suspect click.rich_click.MAX_WIDTH = 180 is somehow involved.

Here is what I get when running breeze from main:
Screenshot 2022-08-01 at 12 40 13 PM

And here is what I get when running breeze from your branch:
Screenshot 2022-08-01 at 12 40 14 PM

I haven't reviewed any of the CI stuff, FYI. Not sure I understand it well enough to contribute much there.

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

Yeah. The 180 is a new feature in rich click -but It's not needed at all. I can remove it. Thanks @rossturk for looking at it :)

@potiuk potiuk force-pushed the move-breeze-commands-into-groups branch from 9a822db to a8ff64e Compare August 1, 2022 18:01
@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

Much better indeed.

Screenshot 2022-08-01 at 20 00 40

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

One thing I am a bit torn - should "advanced command group" be before or after "Basic developer commands" - the nice thing about having "basic" commands at the end is that they are "last" - i.e. possibly easier to find WDYT (those who would look at it :) ?

@ephraimbuddy
Copy link
Contributor

One thing I am a bit torn - should "advanced command group" be before or after "Basic developer commands" - the nice thing about having "basic" commands at the end is that they are "last" - i.e. possibly easier to find WDYT (those who would look at it :) ?

I prefer the way it's arranged now. More like a natural order :)

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

I prefer the way it's arranged now. More like a natural order :)

Yeah. That's why I am on the fence :)

@potiuk potiuk force-pushed the move-breeze-commands-into-groups branch from a8ff64e to a14dd04 Compare August 1, 2022 18:21
@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

BTW. I also reviewed the flags for all commands and made sure that the flags are "classified" properly for each command. There were a few flags that have fallen into default "options" and the only ones we want there some really common options like --verbose, --github-repository and so on.

@potiuk potiuk force-pushed the move-breeze-commands-into-groups branch from a14dd04 to 82587e8 Compare August 1, 2022 18:26
@josh-fell
Copy link
Contributor

I prefer the way it's arranged now. More like a natural order :)

+1

Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Yeah, I think keep the basic commands above the advanced commands. If we can get rich to display it in a brighter/lighter gray to make it stand out a bit, that would also be a subtle indicator that people could key off of.

With breeze ci-image and breeze prod-image, the subcommands also have a -image suffix. This seems a little unnecessary and redundant to me, but it might feel more familiar to people who are transitioning from using the previous commands. Maybe just breeze prod-image build and breeze ci-image build? Same story with breeze prod-image verify-image, breeze ci-image verify-image, and breeze ci-image pull-image. This is just a nitpick, so just something to consider.

@potiuk
Copy link
Member Author

potiuk commented Aug 1, 2022

Yeah, I think keep the basic commands above the advanced commands. If we can get rich to display it in a brighter/lighter gray to make it stand out a bit, that would also be a subtle indicator that people could key off of.

I am not sure about that one, I looked into some rich-click integration for other stuff (finding out if we have not missed some commands) and the formatting is not really configurable. We can distinguish commands from options, but not the "top-level" commands from "command groups" - they are collectively under _COMMANDS :(. Without copy-pasting rich-click code we cannot do much about it, I am afraid, so leaving it as is now is probably good-enough option.

# Default styles
STYLE_OPTION = "bold cyan"
STYLE_SWITCH = "bold green"
STYLE_METAVAR = "bold yellow"
STYLE_METAVAR_APPEND = "dim yellow"
STYLE_METAVAR_SEPARATOR = "dim"
STYLE_HEADER_TEXT = ""
STYLE_FOOTER_TEXT = ""
STYLE_USAGE = "yellow"
STYLE_USAGE_COMMAND = "bold"
STYLE_DEPRECATED = "red"
STYLE_HELPTEXT_FIRST_LINE = ""
STYLE_HELPTEXT = "dim"
STYLE_OPTION_HELP = ""
STYLE_OPTION_DEFAULT = "dim"
STYLE_OPTION_ENVVAR = "dim yellow"
STYLE_REQUIRED_SHORT = "red"
STYLE_REQUIRED_LONG = "dim red"
STYLE_OPTIONS_PANEL_BORDER = "dim"
ALIGN_OPTIONS_PANEL = "left"
STYLE_OPTIONS_TABLE_SHOW_LINES = False
STYLE_OPTIONS_TABLE_LEADING = 0
STYLE_OPTIONS_TABLE_PAD_EDGE = False
STYLE_OPTIONS_TABLE_PADDING = (0, 1)
STYLE_OPTIONS_TABLE_BOX = ""
STYLE_OPTIONS_TABLE_ROW_STYLES = None
STYLE_OPTIONS_TABLE_BORDER_STYLE = None
STYLE_COMMANDS_PANEL_BORDER = "dim"
ALIGN_COMMANDS_PANEL = "left"
STYLE_COMMANDS_TABLE_SHOW_LINES = False
STYLE_COMMANDS_TABLE_LEADING = 0
STYLE_COMMANDS_TABLE_PAD_EDGE = False
STYLE_COMMANDS_TABLE_PADDING = (0, 1)
STYLE_COMMANDS_TABLE_BOX = ""
STYLE_COMMANDS_TABLE_ROW_STYLES = None
STYLE_COMMANDS_TABLE_BORDER_STYLE = None
STYLE_ERRORS_PANEL_BORDER = "red"
ALIGN_ERRORS_PANEL = "left"
STYLE_ERRORS_SUGGESTION = "dim"
STYLE_ABORTED = "red"
MAX_WIDTH = int(getenv("TERMINAL_WIDTH")) if getenv("TERMINAL_WIDTH") else None  # type: ignore
COLOR_SYSTEM = "auto"  # Set to None to disable colors
FORCE_TERMINAL = True if getenv("GITHUB_ACTIONS") or getenv("FORCE_COLOR") or getenv("PY_COLORS") else None

With breeze ci-image and breeze prod-image, the subcommands also have a -image suffix. This seems a little unnecessary and redundant to me, but it might feel more familiar to people who are transitioning from using the previous commands. Maybe just breeze prod-image build and breeze ci-image build? Same story with breeze prod-image verify-image, breeze ci-image verify-image, and breeze ci-image pull-image. This is just a nitpick, so just something to consider.

I like this one. Let me change it :). We have good opportunity to change stuff now.

@blag
Copy link
Contributor

blag commented Aug 1, 2022

Other than those nitpicks, this LGTM. 👍

@potiuk potiuk force-pushed the move-breeze-commands-into-groups branch from 82587e8 to 696c281 Compare August 1, 2022 21:50
Originally Python version of Breeze had only a handful of commands,
while moving from Bash, but we are close to completion of the
transition and the number of commands grew quite a lot bringit it
on par to the commands we used to have in the Bash version of Breeze.

However, only a small subset of the commands is actually useful to
average developer, most of the more advanced commands are used in
specific circumstances (release management, configuring Breeze
once or rebuilding the image or even runing non-interactive test
session are rarely used. Therefore it makes much more sense to
surface the common commands as the top-level commands and move
the less frequent commands to subcommands to move them out from
the main help page.

At the same time, the BREEZE.rst documentation got a little messy
during the move and this is the right time to structure it similarly
to breeze commands:

* prerequisites and installation
* first-time configuration
* regular task description
* more advanced tasks grupped in the same subcommands as in Breeze
* diving deeper into details of Breeze implentation for those
  who wish to understand how Breeze works under-the-hood

Aliases for the common commands that users could already get used to
were created, and deprecation warnings are printed in casee those
commands are used (guiding the user to the new commands to use).

Less frequently used options for shell command are still available
in `shell` but they have been removed from the default command to
remove clutter. You can still used them by explicitly specifying
the `shell` commnd.

Configuration for rich click has been separated out from the
command group implementation to separate packages. This allows
for less problems with circular imports - none of the commands
are needed when rich-click configuration is being prepared, which
happens before main `click` command is parsed, which allows to
run imports of the "code" as late as possible.
@potiuk potiuk force-pushed the move-breeze-commands-into-groups branch from 696c281 to 31fb5ff Compare August 1, 2022 21:54
@potiuk
Copy link
Member Author

potiuk commented Aug 2, 2022

Anyone else? That one looks green :)

@potiuk potiuk merged commit b91fd99 into apache:main Aug 2, 2022
@potiuk potiuk deleted the move-breeze-commands-into-groups branch August 2, 2022 15:21
potiuk added a commit that referenced this pull request Aug 4, 2022
* mOve breeze commands to sub-commands

Originally Python version of Breeze had only a handful of commands,
while moving from Bash, but we are close to completion of the
transition and the number of commands grew quite a lot bringit it
on par to the commands we used to have in the Bash version of Breeze.

However, only a small subset of the commands is actually useful to
average developer, most of the more advanced commands are used in
specific circumstances (release management, configuring Breeze
once or rebuilding the image or even runing non-interactive test
session are rarely used. Therefore it makes much more sense to
surface the common commands as the top-level commands and move
the less frequent commands to subcommands to move them out from
the main help page.

At the same time, the BREEZE.rst documentation got a little messy
during the move and this is the right time to structure it similarly
to breeze commands:

* prerequisites and installation
* first-time configuration
* regular task description
* more advanced tasks grupped in the same subcommands as in Breeze
* diving deeper into details of Breeze implentation for those
  who wish to understand how Breeze works under-the-hood

Aliases for the common commands that users could already get used to
were created, and deprecation warnings are printed in casee those
commands are used (guiding the user to the new commands to use).

Less frequently used options for shell command are still available
in `shell` but they have been removed from the default command to
remove clutter. You can still used them by explicitly specifying
the `shell` commnd.

Configuration for rich click has been separated out from the
command group implementation to separate packages. This allows
for less problems with circular imports - none of the commands
are needed when rich-click configuration is being prepared, which
happens before main `click` command is parsed, which allows to
run imports of the "code" as late as possible.

* Update manage-dags-files.rst

(cherry picked from commit b91fd99)
@potiuk potiuk added this to the Airflow 2.3.4 milestone Aug 5, 2022
@ephraimbuddy ephraimbuddy added type:misc/internal Changelog: Misc changes that should appear in change log changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Aug 15, 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..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants