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 2020/02/18 23:16:24 UTC
[GitHub] [airflow] mik-laj opened a new pull request #7458: [AIRFLOW-6838]
Introduce real subcommands for Breeze
mik-laj opened a new pull request #7458: [AIRFLOW-6838] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458
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 the function, because a large number of global variables makes it very difficult to trace the aapplication behavior.
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
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<sup>*</sup>
- [ ] Unit tests coverage for changes (not needed for documentation changes)
- [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [ ] Relevant documentation is updated including usage instructions.
- [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
<sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
---
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/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-589085051
Will do !
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj closed pull request #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj closed pull request #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-587978552
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588009056
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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588009246
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588507636
> 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588470541
@potiuk I rebased. Can you look at it?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-592525441
Done: https://github.com/apache/airflow/pull/7515
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #7458:
[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588491983
> 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-587988274
> also parsing options won't work either.
Can you say something more about it? I checked, I did found any issues
<img width="1440" alt="Screenshot 2020-02-19 at 02 25 55" src="https://user-images.githubusercontent.com/12058428/74792980-3e8ed280-52bf-11ea-8b55-1745beae606d.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7458:
[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#discussion_r381020442
##########
File path: breeze
##########
@@ -290,7 +290,7 @@ Usage: ${CMDNAME} [COMMAND] [FLAGS] -- <EXTRA_ARGS>
The swiss-knife-army tool for Airflow testings. It allows to perform various test tasks:
- * Enter interactive environment when no command flags are specified (default behaviour)
+ * Run interactive terminal when no command flags are specified (default behaviour)
Review comment:
IT's not terminal. It's the whole environment using integrations, runtime, kubernetes, databases etc. I still think environment is better name,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588491983
> 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
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588015135
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588480323
> I think we should get rid of the short one-letter options for sub-commands.
Removed
> ./breeze <subcommand> --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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #7458:
[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#discussion_r381260365
##########
File path: breeze-complete
##########
@@ -27,23 +27,47 @@ _BREEZE_DEFAULT_DOCKERHUB_USER="apache"
_BREEZE_DEFAULT_DOCKERHUB_REPO="airflow"
_BREEZE_SHORT_OPTIONS="
-h P: B: I: K Z X
+h P: B: I:
M: V:
-s b O N
-v y n C A
+s N
+v y n
r p R L u
-c D: H: e a
-t: d: k x: S: F:
+c D: H:
"
_BREEZE_LONG_OPTIONS="
-help python: backend: integration: start-kind-cluster recreate-kind-cluster stop-kind-cluster
+help python: backend: integration:
kubernetes-mode: kubernetes-version:
-skip-mounting-local-sources build-only build-docs
-verbose assume-yes assume-no toggle-suppress-cheatsheet toggle-suppress-asciiart
+skip-mounting-local-sources
+verbose assume-yes assume-no
force-build-images force-pull-images force-clean-images use-local-cache push-images
-cleanup-images dockerhub-user: dockerhub-repo: initialize-local-virtualenv setup-autocomplete
-test-target: docker-compose: stop-environment execute-command: static-check: static-check-all-files:
+cleanup-images dockerhub-user: dockerhub-repo:
+"
+
+_BREEZE_COMMANDS="
+shell
+O build-docs
+b build-only
+e initialize-local-virtualenv
+a setup-autocomplete
+k stop-environment
+S static-check
+F static-check-all-files
+t test-target
+h help
+K start-kind-cluster
+Z recreate-kind-cluster
+X stop-kind-cluster
+toggle-suppress-cheatsheet
+toggle-suppress-asciiart
+"
+
+_BREEZE_EXTRA_ARG_COMMANDS="
+S static-check
Review comment:
Why do we have them repeated here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #7458: [AIRFLOW-6838][WIP]
Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#issuecomment-588491335
> > I think we should get rid of the short one-letter options for sub-commands.
>
> Removed
they're still 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #7458:
[AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7458: [AIRFLOW-6838][WIP] Introduce real subcommands for Breeze
URL: https://github.com/apache/airflow/pull/7458#discussion_r381545067
##########
File path: breeze-complete
##########
@@ -27,23 +27,47 @@ _BREEZE_DEFAULT_DOCKERHUB_USER="apache"
_BREEZE_DEFAULT_DOCKERHUB_REPO="airflow"
_BREEZE_SHORT_OPTIONS="
-h P: B: I: K Z X
+h P: B: I:
M: V:
-s b O N
-v y n C A
+s N
+v y n
r p R L u
-c D: H: e a
-t: d: k x: S: F:
+c D: H:
"
_BREEZE_LONG_OPTIONS="
-help python: backend: integration: start-kind-cluster recreate-kind-cluster stop-kind-cluster
+help python: backend: integration:
kubernetes-mode: kubernetes-version:
-skip-mounting-local-sources build-only build-docs
-verbose assume-yes assume-no toggle-suppress-cheatsheet toggle-suppress-asciiart
+skip-mounting-local-sources
+verbose assume-yes assume-no
force-build-images force-pull-images force-clean-images use-local-cache push-images
-cleanup-images dockerhub-user: dockerhub-repo: initialize-local-virtualenv setup-autocomplete
-test-target: docker-compose: stop-environment execute-command: static-check: static-check-all-files:
+cleanup-images dockerhub-user: dockerhub-repo:
+"
+
+_BREEZE_COMMANDS="
+shell
+O build-docs
+b build-only
+e initialize-local-virtualenv
+a setup-autocomplete
+k stop-environment
+S static-check
+F static-check-all-files
+t test-target
+h help
+K start-kind-cluster
+Z recreate-kind-cluster
+X stop-kind-cluster
+toggle-suppress-cheatsheet
+toggle-suppress-asciiart
+"
+
+_BREEZE_EXTRA_ARG_COMMANDS="
+S static-check
Review comment:
It's a mistake. Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services