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 approved these changes Jun 14, 2021
@kaxil kaxil added this to the Airflow 2.1.1 milestone Jun 14, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Jun 14, 2021

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.

@@ -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

@uranusjr uranusjr Jun 15, 2021

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

@uranusjr uranusjr Jun 15, 2021

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

@jedcunningham jedcunningham Jun 15, 2021

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
37 of 38 checks passed
@kaxil kaxil deleted the docs_default_templated branch Jun 15, 2021
jhtimmins added a commit to astronomer/airflow that referenced this issue Jun 15, 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)
ashb added a commit that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants