Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-3030] Fix doc of command line interface #3872

Merged
merged 1 commit into from
Sep 10, 2018
Merged

[AIRFLOW-3030] Fix doc of command line interface #3872

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions airflow/bin/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@

log = LoggingMixin().log

DAGS_FOLDER = settings.DAGS_FOLDER

if "BUILDING_AIRFLOW_DOCS" in os.environ:
DAGS_FOLDER = '[AIRFLOW_HOME]/dags'
Copy link
Member

Choose a reason for hiding this comment

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

Could we get around all of these changes by instead setting export AIRFLOW_HOME='$AIRFLOW_HOME' before we build the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what is happening before we build the docs, we export BUILDING_AIRFLOW_DOCS environment variable, DAGS_FOLDER = '[AIRFLOW_HOME]/dags' will be only set when we are building docs

Copy link
Member Author

Choose a reason for hiding this comment

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

After this fix, we would see as below:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if you however prefer $AIRFLOW_HOME/dags instead of [AIRFLOW_HOME]/dags. I have set [AIRFLOW_HOME] because we take this parameter from the config file. But I don't have a preference, so $AIRFLOW_HOME/dags is fine for me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ashb

To answer your question, no setting AIRFLOW_HOME variable won't solve it as we take this value from airflow.cfg, but if we change airflow.cfg before building docs, this solves it for self-hosted docs but still fails for ReadTheDocs .

Copy link
Member

Choose a reason for hiding this comment

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

Then perhaps export AIRFLOW__CORE__DAGS_FOLDER=... then

I feel that not having to change the code for this case just makes things a bit cleaner long term.

Copy link
Member Author

@kaxil kaxil Sep 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Exporting AIRFLOW__CORE__DAGS_FOLDER= would mess up with ReadTheDocs environment as it would change the Dags folder and hence can break it when parsing some modules that use example dags.

We are already using this for apply_defaults decorator:
https://github.com/apache/incubator-airflow/blob/f7fd78b06b24c15af3773b41ae3ecab7e48bb1ea/airflow/utils/decorators.py#L102-L105

And also in future when we might have more such issues where docs would need to be handled differently, I think this would be better than setting environment variables for each instance.



def sigint_handler(sig, frame):
sys.exit(0)
Expand Down Expand Up @@ -133,7 +138,7 @@ def setup_locations(process, pid=None, stdout=None, stderr=None, log=None):

def process_subdir(subdir):
if subdir:
subdir = subdir.replace('DAGS_FOLDER', settings.DAGS_FOLDER)
subdir = subdir.replace('DAGS_FOLDER', DAGS_FOLDER)
subdir = os.path.abspath(os.path.expanduser(subdir))
return subdir

Expand Down Expand Up @@ -1456,8 +1461,10 @@ class CLIFactory(object):
"The regex to filter specific task_ids to backfill (optional)"),
'subdir': Arg(
("-sd", "--subdir"),
"File location or directory from which to look for the dag",
default=settings.DAGS_FOLDER),
"File location or directory from which to look for the dag. "
"Defaults to '[AIRFLOW_HOME]/dags' where [AIRFLOW_HOME] is the "
"value you set for 'AIRFLOW_HOME' config you set in 'airflow.cfg' ",
default=DAGS_FOLDER),
'start_date': Arg(
("-s", "--start_date"), "Override start_date YYYY-MM-DD",
type=parsedate),
Expand Down Expand Up @@ -1874,7 +1881,7 @@ class CLIFactory(object):
"If reset_dag_run option is used,"
" backfill will first prompt users whether airflow "
"should clear all the previous dag_run and task_instances "
"within the backfill date range."
"within the backfill date range. "
"If rerun_failed_tasks is used, backfill "
"will auto re-run the previous failed task instances"
" within the backfill date range.",
Expand Down