Skip to content

[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze#7458

Closed
mik-laj wants to merge 1 commit intoapache:masterfrom
mik-laj:AIRFLOW-6838
Closed

[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze#7458
mik-laj wants to merge 1 commit intoapache:masterfrom
mik-laj:AIRFLOW-6838

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Feb 18, 2020

Breeze currently selects action using many different flags. I know from my own experience that this is very confusing. I think that subcommands will be much easier to use.

I suspect that Breeze autocomplete is not working properly, but I have no experience with this component. Could someone help me with this?

In the next step, I think that we should replace global variables with functions, because a large number of global variables makes it very difficult to trace the aapplication behavior.


Issue link: AIRFLOW-6838

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.
Read the Pull Request Guidelines for more information.

@mik-laj mik-laj changed the title [AIRFLOW-6838] Introduce real subcommands for Breeze [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze Feb 18, 2020
@potiuk
Copy link
Member

potiuk commented Feb 19, 2020

While I like the spirit of that change,. Breeze grew a lot in terms of functionality and maybe we should think of refactoring the structure of commands soon.

But I think it's not the right way of doing it.

First of all you are just about to have a lot of conflicts as we are splitting breeze into functions. Secondly it does not take into account getopt's behaviour so it is for sure wrong (We define all the available options in breeze-complete and getopt by default assumes -- or - params. So not only autocomplete will not work but also parsing options won't work either.

I think we need to do much bolder change if we want to introduce sub-commands - we should rewrite it in another language (python or golang). But complicating parsing of parameters is not a good idea.

Unless you use ready to use library - for example here http://people.tuebingen.mpg.de/maan/gsu/ then we could write it in bash (but it's not a good idea as it is GPL2 and it does not support ZSH I believe). I think we should talk about it and think if that's the right time to think of such change or maybe better to leave it for after we deliver the current important 2.0 features - prod image, API and others.

breeze Outdated
Copy link
Member

Choose a reason for hiding this comment

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

IT's not terminal. It's the whole environment using integrations, runtime, kubernetes, databases etc. I still think environment is better name,

@mik-laj
Copy link
Member Author

mik-laj commented Feb 19, 2020

also parsing options won't work either.

Can you say something more about it? I checked, I did found any issues
Screenshot 2020-02-19 at 02 25 55

@potiuk
Copy link
Member

potiuk commented Feb 19, 2020

Sure: getopt verifies switches provided and will complain if the switch is wrong. Try --nnn . with the command specified you get no verification you can do ./breeeze nnnn and it will do something instead of complaining. If you specify --stop-environment as switch, it will pass over getopt but --stop-environments will fail in get_opt. And the list of options (available in breeze-complete) is not updated. The same list is used for autocomplete

@potiuk
Copy link
Member

potiuk commented Feb 19, 2020

Even if you provide --stop, get opt will suggest that it is ambigius and will tell you that --stop-environment is one of possible options.

@mik-laj
Copy link
Member Author

mik-laj commented Feb 19, 2020

I updated autocompllete. This should also solve the problems with getopt.

you can do ./breeeze nnnn and it will do something instead of complaining.

Thanks. I added missing check.

I still have to do rebase, but I will do it tomorrow.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

This is much better now. I really like where it goes.

Two general comments:

  • I think we should get rid of the short one-letter options for sub-commands. It will make it a bit smaller and I think it makes much more sense in the context of this change. I've never seen one letter commands for subcommands in any other tool

  • we should behave similarly as other tools using subcommands:
    ./breeze --help should return only list of subcommands with short description, common options for all commands
    ./breeze <subcommand> --help should return detailed explanation for that command only

I am happy to work together with you on that and contribute a commit or two to add it after you rebase your changes.

@mik-laj
Copy link
Member Author

mik-laj commented Feb 19, 2020

@potiuk I rebased. Can you look at it?

@mik-laj
Copy link
Member Author

mik-laj commented Feb 19, 2020

I think we should get rid of the short one-letter options for sub-commands.

Removed

./breeze --help should return detailed explanation for that command only

I agree, but I think this is an additional function that we can do independently. This will require much more code changes. In my opinion, we can divide it into two steps. First enter the commands and check users' opinions about them, and then refactor the code to further improve the code.

This is a big change, because it will also require that we do not have one parser, but several smaller parsers.

@potiuk
Copy link
Member

potiuk commented Feb 19, 2020

I think we should get rid of the short one-letter options for sub-commands.

Removed

they're still there.

@potiuk
Copy link
Member

potiuk commented Feb 19, 2020

This is a big change, because it will also require that we do not have one parser, but several smaller parsers.

Not necessary. that's just splitting help - nothing else. One parser should be enough. Again - happy to add it.

@mik-laj
Copy link
Member Author

mik-laj commented Feb 19, 2020

they're still there.

pre-commit got stuck when asking to rebuild the image. I pushed the changes now.

Not necessary. that's just splitting help - nothing else. One parser should be enough. Again - happy to add it.

I invite you to contribute.

@potiuk
Copy link
Member

potiuk commented Feb 20, 2020

Will do !

@mik-laj
Copy link
Member Author

mik-laj commented Feb 28, 2020

Done: #7515

@mik-laj mik-laj closed this Feb 28, 2020
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.

2 participants