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

Add Airflow UI instance_name configuration option #10162

Merged
merged 6 commits into from
Feb 17, 2021

Conversation

pcandoalmeida
Copy link
Contributor

Description

Site title configuration added for DAGs home page. This will allow multiple installations of Airflow to be distinguished via the UI and page title.

Logic added to view.py and dags.html connecting a configuration change in airflow.cfg to replace the default DAGs header and Airflow - DAGs title into a custom site_title string.

  • Unit and integration tests added
  • Documentation added
  • Pre-commit checks

Closes: #9538

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Aug 4, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 4, 2020

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://apache-airflow-slack.herokuapp.com/

airflow/www/views.py Outdated Show resolved Hide resolved
@mik-laj
Copy link
Member

mik-laj commented Aug 5, 2020

Could you please add some documentation? I am thinking about a new page in the howto directory.
https://airflow.readthedocs.io/en/latest/howto/index.html
In my next PR, I am trying to clean up the documentation to bring together several related UI related documentation pages, but for now the new page is the best solution.

@pcandoalmeida
Copy link
Contributor Author

Could you please add some documentation? I am thinking about a new page in the howto directory.
https://airflow.readthedocs.io/en/latest/howto/index.html
In my next PR, I am trying to clean up the documentation to bring together several related UI related documentation pages, but for now the new page is the best solution.

Sure, I added site_title to the configuration reference page, but I can add a new page to the directory.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

Can you also post before and after screenshot of the homepage

@pcandoalmeida
Copy link
Contributor Author

Can you also post before and after screenshot of the homepage

Sure @kaxil I've added an after screenshot, but I will do a before too.

airflow/www/views.py Outdated Show resolved Hide resolved
docs/howto/customize-dag-ui-page-site-title.rst Outdated Show resolved Hide resolved
tests/test_configuration.py Outdated Show resolved Hide resolved
tests/test_configuration.py Outdated Show resolved Hide resolved
@mik-laj
Copy link
Member

mik-laj commented Aug 6, 2020

What do you think about giving the ability to change adding a unique title name for all the window?
Screenshot 2020-08-06 at 00 57 54
I think that if site_title is set it can replace the word Airflow.

@pcandoalmeida
Copy link
Contributor Author

What do you think about giving the ability to change adding a unique title name for all the window?
Screenshot 2020-08-06 at 00 57 54
I think that if site_title is set it can replace the word Airflow.

Hi @mik-laj it should replace the entire page title. I will have a look into this as when I tested it via Breeze, it did so.

@mik-laj
Copy link
Member

mik-laj commented Aug 6, 2020

you only make changes to the DAG list. I think that this change is worth introducing on all views.

@pcandoalmeida
Copy link
Contributor Author

Hi @mik-laj I've got this working for a couple of test views (there's quite few!). Would I need to add unit tests for every view? I would assume so, but wanted to ask so I can do it as I go along.

@pcandoalmeida
Copy link
Contributor Author

Hi everyone, not sure if someone might be able to provide some guidance. I've managed to update DAG related views, like the graph and code views for example. Been digging around the code, but I can't quite figure out how to amend views like Browser > DAG Runs. I think I might have to amend the code a bit to get it working, but not quite sure. Any advice/thoughts would be greatly appreciated.

@pcandoalmeida pcandoalmeida requested review from mik-laj and ashb August 16, 2020 11:39
@mik-laj
Copy link
Member

mik-laj commented Sep 4, 2020

@pcandoalmeida I prepared the patch and push it on your branch. Can you check it?

@pcandoalmeida
Copy link
Contributor Author

Sure @mik-laj I'll have a look tomorrow!

@pcandoalmeida
Copy link
Contributor Author

Thanks @mik-laj I'm reviewing your updates now; let me know how you want to proceed. Can close this issue if you want to go with the other update.

@mik-laj
Copy link
Member

mik-laj commented Sep 15, 2020

@pcandoalmeida I support this change, but it is helpful if you rebase your branch to the newest master and fix all failed tests.

@pcandoalmeida
Copy link
Contributor Author

Sure @mik-laj doing that now.

@pcandoalmeida
Copy link
Contributor Author

Hi @mik-laj I've resolved about 16 failing tests in www/. I'm having a strange Github issue when trying to rebase, but will update the PR soon.

version_added: ~
type: string
example: ~
default:
Copy link
Member

Choose a reason for hiding this comment

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

We could actually add the default here, so that users can easily override it.

Suggested change
default:
default: Airflow

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts here @pcandoalmeida ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hiya @kaxil sorry didn't see this! Yes, this defaults the page title to Airflow. By default, we have Airflow - Airflow unless there is a specific instance name like Airflow - Dev.

@@ -1080,6 +1080,13 @@
type: string
example: ~
default: "30"
- name: site_title
description: |
Sets a title to show in the browser tab and the DAGs overview page
Copy link
Member

Choose a reason for hiding this comment

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

This comment needs updating now -- it's all pages, isn't it?

@@ -44,6 +44,7 @@ def init_jinja_globals(app):

def prepare_jinja_globals():
extra_globals = {
'app_name': conf.get(section="webserver", key="site_title", fallback="Airflow"),
Copy link
Member

Choose a reason for hiding this comment

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

We could do this without an extra variable, using {{ appbuilder.app_name }} in the templates (which will already be set up for us)

Copy link
Contributor Author

@pcandoalmeida pcandoalmeida Sep 17, 2020

Choose a reason for hiding this comment

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

Hi @ashb just wanted to double check: I've removed app_name in Line:47 and have replaced app_name in the relevant *.html files with {{ appbuilder.app_name }}. Just wanted to make sure I understood correctly.

@@ -34,7 +34,7 @@
{% endblock %}

{% block content %}
<h2>DAGs</h2>
<h2>{{ site_title }}</h2>
Copy link
Member

@ashb ashb Sep 17, 2020

Choose a reason for hiding this comment

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

This one is wrong - it should still include/be DAGs.

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 think the original issue has evolved via @mik-laj's suggestion to update every page. Initially the request was to be able to amend the page title in dags.html so to distinguish differing installatios via the UI.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see

@github-actions
Copy link

The Workflow run is cancelling this PR. Building image for the PR has been cancelled

@pcandoalmeida pcandoalmeida force-pushed the 9538-Airflow-UI-Site-Title branch 2 times, most recently from d50e511 to dbedad4 Compare October 10, 2020 22:17
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

LGTM but I think the screenshots need to be updated 👍

@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@turbaszek turbaszek requested a review from kaxil October 11, 2020 23:18
@mik-laj mik-laj removed their request for review October 12, 2020 11:18
@pcandoalmeida
Copy link
Contributor Author

Hi @turbaszek @kaxil 👋🏼 We have a Hacktoberfest day coming up this Friday at work; do let me know if you have any feedback on this and I can work on it during the day! Hopefully we're close to pushing this one over the line! 🚀

@@ -515,11 +515,14 @@ def get_int_arg(value, default=0):
state_color_mapping = State.state_color.copy()
state_color_mapping["null"] = state_color_mapping.pop(None)

page_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. same config with a different fallback?

Copy link
Member

Choose a reason for hiding this comment

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

fallback only works if config is not provided, when config is provided DAGs won't be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same config with a different fallback?

So amend the fallback? I think if the site_title config is set, then we use that; else, we default to the standard DAGs page title in the DAGs UI page.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe have 2 different configs, 1 for page_title and 1 for site_title unless this has already been discussed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yh I did ask about this earlier on, but I think based on what Kamil said we integrated the changes?

Copy link
Member

@mik-laj mik-laj Oct 16, 2020

Choose a reason for hiding this comment

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

I think we can use a more generic option name like instance_name which will be empty by default. When it is set, some strings will be replaced with the name of the instance, and in other cases, the description will be used more appropriately to the case. I don't see the benefit of splitting this into two options if the user always sets the same value in the two options. We want to get it to be able to easily recognize different environments/instances of this application, so we can use an option name that describes 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.

Thanks both; OK so basically just rename site_title to instance_name? The default behaviour for the UI will be DAGs and the page title Airflow. I'll amend the code and docs.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@mik-laj
Copy link
Member

mik-laj commented Dec 22, 2020

@pcandoalmeida What is the status of this change? Do you need help?

@pcandoalmeida
Copy link
Contributor Author

Hi @mik-laj I think I made the changes requested by Kaxil and Tomek, but had some build errors (wasn't too sure why as I think it related to a test version error). I'll need to review this and fix the conflicts I think, unless something else has come up in the interim. Hopefully this should be near the end!

@ashb ashb force-pushed the 9538-Airflow-UI-Site-Title branch from a95282f to d31577c Compare February 15, 2021 14:12
@github-actions
Copy link

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb ashb changed the title Add Airflow UI site_title configuration option Add Airflow UI instance_name configuration option Feb 16, 2021
@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb ashb merged commit 7d60bbf into apache:master Feb 17, 2021
@ashb ashb added this to the Airflow 2.1 milestone Feb 17, 2021
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.

Add config variable for UI page title
6 participants