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.

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

Fix templated default/example values in config ref docs #16442

Merged
merged 1 commit into from Jun 15, 2021

Conversation

jedcunningham
Copy link
Member

@jedcunningham jedcunningham commented Jun 14, 2021

We should show the actual default/example value in the configuration
reference docs, not the templated values.

e.g. {dag_id} like you get in a generated airflow.cfg, not {{dag_id}}
like is stored in the airflow.cfg template.

Before:
Screen Shot 2021-06-14 at 4 11 36 PM

After:
Screen Shot 2021-06-14 at 4 11 45 PM

We should show the actual default/example value in the configuration
reference docs, not the templated values.

e.g. `{dag_id}` like you get in a generated airflow.cfg, not `{{dag_id}}
like is stored in the airflow.cfg template.
@kaxil kaxil added this to the Airflow 2.1.1 milestone Jun 14, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 14, 2021
@@ -92,7 +92,7 @@ def _default_config_file_path(file_name: str):
return os.path.join(templates_dir, file_name)


def default_config_yaml() -> dict:
def default_config_yaml() -> List[dict]:
Copy link
Member

Choose a reason for hiding this comment

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

Is this right? PyYAML documentation says safe_load only loads the first document, which would be dict.

Copy link
Member

Choose a reason for hiding this comment

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

https://pyyaml.org/wiki/PyYAMLDocumentation

safe_load(stream) parses the given stream and returns a Python object constructed from for the first document in the stream. If there are no documents in the stream, it returns None. safe_load recognizes only standard YAML tags and cannot construct an arbitrary Python object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it is a list:
https://github.com/apache/airflow/blob/main/airflow/config_templates/config.yml

PyYAML is referring to YAML documents: https://pyyaml.org/wiki/PyYAMLDocumentation#documents

We only have one, but you could have more than one in the steam, e.g:

---
- a:b
---
- c:d

Interestingly, for me safe_load actually raises when it sees more than 1 document 🤷‍♂️

@kaxil kaxil merged commit cc3c13c into apache:main Jun 15, 2021
@kaxil kaxil deleted the docs_default_templated branch June 15, 2021 16:34
ashb pushed a commit that referenced this pull request Jun 22, 2021
We should show the actual default/example value in the configuration
reference docs, not the templated values.

e.g. `{dag_id}` like you get in a generated airflow.cfg, not `{{dag_id}}
like is stored in the airflow.cfg template.

(cherry picked from commit cc3c13c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants