Skip to content

Flag --start-airflow for breeze#10837

Merged
potiuk merged 4 commits intoapache:masterfrom
francescomucio:master
Sep 11, 2020
Merged

Flag --start-airflow for breeze#10837
potiuk merged 4 commits intoapache:masterfrom
francescomucio:master

Conversation

@francescomucio
Copy link
Contributor

As a DAG developer I want to have a simple way to run Airflow and monitor what is going on with my DAGs.

My normal workflow would be to start breeze and then open a couple of panes with tmux to start the scheduler and the webserver. Sometimes I need to initialize the db and create a dummy admin user to login.

This will be automated with the additional flag --start-airflow. The tmux session will be initialized with the three panes and the Airflow Scheduler and Webserver will start automatically.


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

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.

I think this is a fantastic feature!

Just a few nits and one typo by mistake I guess.

@potiuk
Copy link
Member

potiuk commented Sep 9, 2020

There are some (good) advices from our shelcheck pre-commit (Static checks failing). You should take a look @francescomucio :)

@potiuk
Copy link
Member

potiuk commented Sep 10, 2020

@francescomucio -> I strongly recommend installing pre-commit locally (see STATIC_CODE_CHECKS.rst) . Then all those things will be either fixed automatically or reported locally while you are committing it.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you think it's not needed ? I think where you use postgres and mysql backend, this is quite likely that the database is still running and the user is already created. I think this could be only removed for sqlite backend, but we should also support mysql/postgres

Copy link
Member

Choose a reason for hiding this comment

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

You can try this by running ./breeze --start-airflow --backend mysql exitng and re-running it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is not needed because if db init is executed again no data are lost and trying to create the Admin user again also will not raise and error (just the message that the user Admin is already there).

That said I spent some time trying to figure out the best way to check if the database was already initialized or not, just to discover that that check is already in the db init command :)

Copy link
Member

Choose a reason for hiding this comment

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

Let me just double check and we can merge :)

Copy link
Member

Choose a reason for hiding this comment

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

All good!

Copy link
Member

Choose a reason for hiding this comment

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

Can you please rebase as @mik-laj asked ? This should fix the spelling problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


cassandra kerberos mongo openldap presto rabbitmq redis all

--start-airflow
Copy link
Member

Choose a reason for hiding this comment

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

Most other options like this area command, not a flag, so this should be breeze start-airflow for consistency I think

Copy link
Member

Choose a reason for hiding this comment

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

I had exactly the same thought initially, but after a bit of thought - I look at this as an option of "shell" command (this is the default command when none is specified) - it is much more similar to --db-reset flag, --db-reset means "reset DB when you enter the shell" and --start-airflow means "start airflow when you enter the shell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, it makes sense for me.
How should I proceed?

Copy link
Member

@potiuk potiuk Sep 11, 2020

Choose a reason for hiding this comment

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

But when I think about it now - this might be looked at as indeed pretty "prominent" way of using Airflow so it might deserve it's own command WDYT @ashb @turbaszek @francescomucio ?

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was that since this doesn't couple work any of the existing commands (I.e. breeze _anything_ --start-airflow doesn't make sense) that it is a command, not a flag.

Copy link
Member

Choose a reason for hiding this comment

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

OK some guidelines then @francescomucio if you want to give a shot at that -> look how "shell is defined for example - it should set the "command_to_run" to a new command ("start_airflow"?) and then you can look for "enter_breeze" command and implement it similarly - but only for CI image (there is no tmux in prod image).

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't direct most end users to this (as it stands) - as it will run main branch, not a released version

True. The "prominent" in this case means being a separate command rather than option. Do you think it's better to leave it as a shell option then? For me - I can go with both, but --start-airflow does indicate a bit more "developer'y" option rather than "first class command".

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not sure anymore either :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge it as is - we can change later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for your support guys, it was fun for me to work on this :)

@potiuk potiuk merged commit 47e592e into apache:master Sep 11, 2020
@potiuk
Copy link
Member

potiuk commented Sep 11, 2020

Thanks @francescomucio !

potiuk pushed a commit to potiuk/airflow that referenced this pull request Sep 14, 2020
potiuk pushed a commit that referenced this pull request Sep 15, 2020
RaviTezu pushed a commit to RaviTezu/airflow that referenced this pull request Oct 25, 2020
kaxil pushed a commit that referenced this pull request Nov 12, 2020
@potiuk potiuk added this to the Airflow 1.10.13 milestone Nov 14, 2020
@potiuk potiuk added the type:misc/internal Changelog: Misc changes that should appear in change log label Nov 14, 2020
potiuk pushed a commit that referenced this pull request Nov 16, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants