Skip to content

Add a default for DagModel.default_view#10897

Merged
kaxil merged 1 commit intoapache:masterfrom
astronomer:fix-dag-default-view-bug
Sep 15, 2020
Merged

Add a default for DagModel.default_view#10897
kaxil merged 1 commit intoapache:masterfrom
astronomer:fix-dag-default-view-bug

Conversation

@kaxil
Copy link
Copy Markdown
Member

@kaxil kaxil commented Sep 12, 2020

fixes #10283


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

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Sep 12, 2020
Comment on lines +1624 to +1629
def get_default_view(self):
"""This is only there for backward compatible jinja2 templates"""
if self.default_view is None:
return conf.get('webserver', 'dag_default_view').lower()
else:
return self.default_view
Copy link
Copy Markdown
Member

@mik-laj mik-laj Sep 15, 2020

Choose a reason for hiding this comment

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

Suggested change
def get_default_view(self):
"""This is only there for backward compatible jinja2 templates"""
if self.default_view is None:
return conf.get('webserver', 'dag_default_view').lower()
else:
return self.default_view
@property
def default_view(self):
"""This is only there for backward compatible jinja2 templates"""
if self._default_view is None:
return conf.get('webserver', 'dag_default_view').lower()
else:
return self._default_view
@default_view.setter
def default_view(self, value):
self._default_view= value

This way we won't have two similar public attributes of default_view property, get_default_view method.

Copy link
Copy Markdown
Member Author

@kaxil kaxil Sep 15, 2020

Choose a reason for hiding this comment

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

I kept it there for parity between Dag and DagModel, otherwise dag.html and dags.html might use or the other depending on if we are using Dag or DagModel

The other option is to create a Database migration to fill all the null values, WDYT? @mik-laj

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mik-laj Any comments?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okey. Now I understand the whole change. It looks good.

@kaxil kaxil merged commit 905cdd5 into apache:master Sep 15, 2020
@kaxil kaxil deleted the fix-dag-default-view-bug branch September 15, 2020 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Airflow Homepage is not able to render due to TypeError: must be str, not NoneType

3 participants