You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/04 22:58:22 UTC
[GitHub] [airflow] pcandoalmeida opened a new pull request #10162: Add Airflow UI site_title configuration option
pcandoalmeida opened a new pull request #10162:
URL: https://github.com/apache/airflow/pull/10162
**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.
- [x] Unit and integration tests added
- [x] Documentation added
- [x] Pre-commit checks
Closes: #9538
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669975576
What do you think about giving the ability to change adding a unique title name for all the window?
<img width="285" alt="Screenshot 2020-08-06 at 00 57 54" src="https://user-images.githubusercontent.com/12058428/89546233-a781ec80-d804-11ea-9782-6b24d24fff5e.png">
I think that if site_title is set it can replace the word Airflow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490444363
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
I'll double check this, but if not needed for the UI will remove.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r502785541
##########
File path: scripts/ci/static_checks/ci_pylint_tests.sh
##########
@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+export PYTHON_MAJOR_MINOR_VERSION=${PYTHON_MAJOR_MINOR_VERSION:-3.6}
+
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+function run_pylint_tests() {
+ FILES=("$@")
+ if [[ "${#FILES[@]}" == "0" ]]; then
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" \
+ | tee -a "${OUTPUT_LOG}"
+ else
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" "${FILES[@]}" \
+ | tee -a "${OUTPUT_LOG}"
+ fi
+}
+
+get_environment_for_builds_on_ci
+
+prepare_ci_build
+
+rebuild_ci_image_if_needed
+
+if [[ "${#@}" != "0" ]]; then
+ filter_out_files_from_pylint_todo_list "$@"
+
+ if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then
+ echo "Filtered out all files. Skipping pylint."
+ else
+ run_pylint_tests "${FILTERED_FILES[@]}"
+ fi
+else
+ run_pylint_tests
+fi
Review comment:
No, it's been open for a while; yep will rebase now ππΌ
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490453209
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
Have amended the code slightly so that `Airflow - DAGs - CustomText` appears as the site_title and the `<h2>` renders the page_title as per the original suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466037021
##########
File path: tests/www/test_views.py
##########
@@ -1022,6 +1022,18 @@ def log_name(self):
self.assertTrue(ctx['show_external_log_redirect'])
self.assertEqual(ctx['external_log_name'], ExternalHandler.LOG_NAME)
+ def test_page_site_title(self):
+ with conf_vars({('webserver', 'site_title'): 'Site Title Test'}):
+ resp = self.client.get('home', follow_redirects=True)
+ self.check_content_in_response('Site Title Test', resp)
+
+ def test_page_site_title_xss_prevention(self):
+ xss_string = "<script>alert('Give me your credit card number')</script>"
+ with conf_vars({('webserver', 'site_title'): xss_string}):
+ resp = self.client.get('home', follow_redirects=True)
+ escaped_xss_string = "<script>alert(Give me your credit card number)</script>"
+ self.check_content_in_response(escaped_xss_string, resp)
Review comment:
```suggestion
self.check_content_in_response(escaped_xss_string, resp)
self.check_content_not_in_response(xss_string, resp)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490162592
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
Why do we need both?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490454284
##########
File path: airflow/www/templates/airflow/master.html
##########
@@ -19,6 +19,14 @@
{% extends 'appbuilder/baselayout.html' %}
+{% block page_title -%}
+ {% if title is defined -%}
+ {{ title }} - {{ app_name }}
Review comment:
Would this be defined in `master.html` @ashb? I think this might be the more robust option, as right now I have modified several files to update the `site_title` variable. It works nicely as is, but the above is cleaner I feel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490425574
##########
File path: airflow/www/extensions/init_jinja_globals.py
##########
@@ -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"),
Review comment:
Hi @ashb just wanted to double check: I've removed `app_name` in Line:47 and have replaced `app_name` in the *.html files with `{{ appbuilder.app_name }}`. Just wanted to make sure I understood correctly.
##########
File path: airflow/www/extensions/init_jinja_globals.py
##########
@@ -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"),
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r483872791
##########
File path: airflow/config_templates/config.yml
##########
@@ -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
+ version_added: ~
+ type: string
+ example: ~
+ default: "DAGs"
Review comment:
```suggestion
default:
```
We will have more freedom if we do not define a default value. The default value can be specified while reading.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ektormak commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ektormak commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r465609170
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
+ page_title = site_title
Review comment:
why not use 1 variable here since they have the same value?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-706778093
[The Workflow run](https://github.com/apache/airflow/actions/runs/301018873) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490453209
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
Have amended the code slightly so that `Airflow - DAGs - CustomText` appears as the site_title and the `<h2>` renders the page_title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-779313585
[The Workflow run](https://github.com/apache/airflow/actions/runs/569036332) 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*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-670724808
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506375181
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
fallback only works if config is not provided, when config is provided `DAGs` won't be used
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ektormak commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ektormak commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r465609170
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
+ page_title = site_title
Review comment:
why not use 1 variable here since they have the same value?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r465829998
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
Review comment:
Yes, could do. I think it might help with readability if perhaps this is defined close to the logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490161787
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
Review comment:
This block/condition doesn't make sense given the new default is "Airflow".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490159931
##########
File path: airflow/www/extensions/init_jinja_globals.py
##########
@@ -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"),
Review comment:
We could do this without an extra variable, using `{{ appbuilder.app_name }}` in the templates (which will already be set up for us)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490160211
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -34,7 +34,7 @@
{% endblock %}
{% block content %}
- <h2>DAGs</h2>
+ <h2>{{ site_title }}</h2>
Review comment:
This one is wrong - it should still include/be DAGs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-706547003
[The Workflow run](https://github.com/apache/airflow/actions/runs/299172344) is cancelling this PR. Building image for the PR has been cancelled
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506373859
##########
File path: airflow/config_templates/config.yml
##########
@@ -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
+ version_added: ~
+ type: string
+ example: ~
+ default:
Review comment:
Thoughts here @pcandoalmeida ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490413605
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
As above: to have a way to distinguish an installation from the main DAGs page title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466466928
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
Review comment:
I tested it yesterday before I went to sleep. It is not necessary.
<img width="542" alt="Screenshot 2020-08-06 at 00 54 02" src="https://user-images.githubusercontent.com/12058428/89546022-5f62ca00-d804-11ea-90ea-6357694d04ab.png">
<img width="574" alt="Screenshot 2020-08-06 at 00 54 36" src="https://user-images.githubusercontent.com/12058428/89546028-612c8d80-d804-11ea-99d6-a15f5a9f67a4.png">
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506445230
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
Maybe have 2 different configs, 1 for page_title and 1 for site_title unless this has already been discussed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669982228
you only make changes to the DAG list. I think that this change is worth on all views.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r502784694
##########
File path: scripts/ci/static_checks/ci_pylint_tests.sh
##########
@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+export PYTHON_MAJOR_MINOR_VERSION=${PYTHON_MAJOR_MINOR_VERSION:-3.6}
+
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+function run_pylint_tests() {
+ FILES=("$@")
+ if [[ "${#FILES[@]}" == "0" ]]; then
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" \
+ | tee -a "${OUTPUT_LOG}"
+ else
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" "${FILES[@]}" \
+ | tee -a "${OUTPUT_LOG}"
+ fi
+}
+
+get_environment_for_builds_on_ci
+
+prepare_ci_build
+
+rebuild_ci_image_if_needed
+
+if [[ "${#@}" != "0" ]]; then
+ filter_out_files_from_pylint_todo_list "$@"
+
+ if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then
+ echo "Filtered out all files. Skipping pylint."
+ else
+ run_pylint_tests "${FILTERED_FILES[@]}"
+ fi
+else
+ run_pylint_tests
+fi
Review comment:
Is this related to the PR? Possibly you may need a rebase π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669577449
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490412036
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -34,7 +34,7 @@
{% endblock %}
{% block content %}
- <h2>DAGs</h2>
+ <h2>{{ site_title }}</h2>
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506498927
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
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 really want to get it to be able to easily recognize different environments/instances of this application.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-693019071
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r502784694
##########
File path: scripts/ci/static_checks/ci_pylint_tests.sh
##########
@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+export PYTHON_MAJOR_MINOR_VERSION=${PYTHON_MAJOR_MINOR_VERSION:-3.6}
+
+# shellcheck source=scripts/ci/libraries/_script_init.sh
+. "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+
+function run_pylint_tests() {
+ FILES=("$@")
+ if [[ "${#FILES[@]}" == "0" ]]; then
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" \
+ | tee -a "${OUTPUT_LOG}"
+ else
+ docker run "${EXTRA_DOCKER_FLAGS[@]}" \
+ --entrypoint "/usr/local/bin/dumb-init" \
+ "${AIRFLOW_CI_IMAGE}" \
+ "--" "/opt/airflow/scripts/ci/in_container/run_pylint_tests.sh" "${FILES[@]}" \
+ | tee -a "${OUTPUT_LOG}"
+ fi
+}
+
+get_environment_for_builds_on_ci
+
+prepare_ci_build
+
+rebuild_ci_image_if_needed
+
+if [[ "${#@}" != "0" ]]; then
+ filter_out_files_from_pylint_todo_list "$@"
+
+ if [[ "${#FILTERED_FILES[@]}" == "0" ]]; then
+ echo "Filtered out all files. Skipping pylint."
+ else
+ run_pylint_tests "${FILTERED_FILES[@]}"
+ fi
+else
+ run_pylint_tests
+fi
Review comment:
Is this related to the PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506514906
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ektormak commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ektormak commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r465608498
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
Review comment:
why not add this to the constants in the module level scope?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490413605
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
As above: to have a way to distinguish an installation from the main DAGs page title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466464765
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
Review comment:
Just this line where we remove quotes
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506606192
##########
File path: airflow/config_templates/config.yml
##########
@@ -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
+ version_added: ~
+ type: string
+ example: ~
+ default:
Review comment:
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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-687479611
Sure @mik-laj I'll have a look tomorrow!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #10162: Add Airflow UI instance_name configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-779973825
[The Workflow run](https://github.com/apache/airflow/actions/runs/572272871) 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*.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466464943
##########
File path: tests/test_configuration.py
##########
@@ -658,3 +658,16 @@ def test_store_dag_code_config_when_set(self):
self.assertTrue(conf.has_option("core", "store_dag_code"))
self.assertTrue(store_serialized_dags)
self.assertFalse(store_dag_code)
+
+ @conf_vars({("webserver", "site_title"): "True"})
+ def test_site_title_config_when_set(self):
+ site_title_config = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ self.assertTrue(conf.has_option("webserver", "site_title"))
+ self.assertTrue(bool(site_title_config))
+
+ @conf_vars({("webserver", "site_title"): None})
+ def test_site_title_config_when_not_set(self):
Review comment:
Yes please
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506429833
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466464148
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
Review comment:
Hi @ashb thanks for the feedback here. Remove lines 339 - 342 or something in particular?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466462981
##########
File path: docs/howto/customize-dag-ui-page-site-title.rst
##########
@@ -0,0 +1,55 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+Customizing DAG UI header and page titles
+=========================================
+
+Airflow now allows you to customise the DAG home page header and page title. This will help
+distinguish between various installations of Airflow or simply amend the page text.
+
+Note: the custom title will be applied to both the page header and the page title.
+
+To make this change, simply:
+
+1. Add the configuration option of ``site_title`` under ``webserver`` inside ``airflow.cfg``:
+
+.. code-block::
+
+ [webserver]
+
+ site_title = "Text from airflow.cfg"
Review comment:
Done ππΌ
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-692973693
@pcandoalmeida I support this change, but it is helpful if you rebase your branch to the newest master and fix all failed tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-670224950
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490161213
##########
File path: airflow/www/templates/airflow/master.html
##########
@@ -19,6 +19,14 @@
{% extends 'appbuilder/baselayout.html' %}
+{% block page_title -%}
+ {% if title is defined -%}
+ {{ title }} - {{ app_name }}
Review comment:
Isn't this going to show "DAGs - Airflow - Airflow"? for the task.html template?
It sounds like it is probably worth defining a `title` block in every page, that way we don't need the app_name in all of them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466428729
##########
File path: tests/test_configuration.py
##########
@@ -658,3 +658,16 @@ def test_store_dag_code_config_when_set(self):
self.assertTrue(conf.has_option("core", "store_dag_code"))
self.assertTrue(store_serialized_dags)
self.assertFalse(store_dag_code)
+
+ @conf_vars({("webserver", "site_title"): "True"})
+ def test_site_title_config_when_set(self):
+ site_title_config = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ self.assertTrue(conf.has_option("webserver", "site_title"))
+ self.assertTrue(bool(site_title_config))
+
+ @conf_vars({("webserver", "site_title"): None})
+ def test_site_title_config_when_not_set(self):
Review comment:
Not need either
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
Review comment:
This shouldn't be needed.
##########
File path: tests/test_configuration.py
##########
@@ -658,3 +658,16 @@ def test_store_dag_code_config_when_set(self):
self.assertTrue(conf.has_option("core", "store_dag_code"))
self.assertTrue(store_serialized_dags)
self.assertFalse(store_dag_code)
+
+ @conf_vars({("webserver", "site_title"): "True"})
+ def test_site_title_config_when_set(self):
Review comment:
This year isn't needed - it is host testing the config loading which is already well tested
##########
File path: docs/howto/customize-dag-ui-page-site-title.rst
##########
@@ -0,0 +1,55 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+Customizing DAG UI header and page titles
+=========================================
+
+Airflow now allows you to customise the DAG home page header and page title. This will help
+distinguish between various installations of Airflow or simply amend the page text.
+
+Note: the custom title will be applied to both the page header and the page title.
+
+To make this change, simply:
+
+1. Add the configuration option of ``site_title`` under ``webserver`` inside ``airflow.cfg``:
+
+.. code-block::
+
+ [webserver]
+
+ site_title = "Text from airflow.cfg"
Review comment:
Remove quotes here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-687469613
@pcandoalmeida I prepared the patch and push it on your branch. Can you check it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490451478
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -34,7 +34,7 @@
{% endblock %}
{% block content %}
- <h2>DAGs</h2>
+ <h2>{{ site_title }}</h2>
Review comment:
Ahh I see
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-749619437
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506490934
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
Yh I did ask about this earlier on, but I think based on what Kamil said we integrated the changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466040341
##########
File path: tests/www/test_views.py
##########
@@ -1022,6 +1022,18 @@ def log_name(self):
self.assertTrue(ctx['show_external_log_redirect'])
self.assertEqual(ctx['external_log_name'], ExternalHandler.LOG_NAME)
+ def test_page_site_title(self):
+ with conf_vars({('webserver', 'site_title'): 'Site Title Test'}):
+ resp = self.client.get('home', follow_redirects=True)
+ self.check_content_in_response('Site Title Test', resp)
+
+ def test_page_site_title_xss_prevention(self):
+ xss_string = "<script>alert('Give me your credit card number')</script>"
+ with conf_vars({('webserver', 'site_title'): xss_string}):
+ resp = self.client.get('home', follow_redirects=True)
+ escaped_xss_string = "<script>alert(Give me your credit card number)</script>"
+ self.check_content_in_response(escaped_xss_string, resp)
Review comment:
Thanks @mik-laj -- good shout. I've committed this suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-708527961
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! π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490413605
##########
File path: airflow/www/views.py
##########
@@ -356,11 +356,18 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ page_title = site_title
+
return self.render_template(
'airflow/dags.html',
dags=dags,
current_page=current_page,
search_query=arg_search_query if arg_search_query else '',
+ page_title=page_title,
Review comment:
As above: to have a way to distinguide an installation from the main DAGs page title.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669982228
you only make changes to the DAG list. I think that this change is worth introducing on all views.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506374740
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
hmm.. same config with a different fallback?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466485188
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
+ site_title = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ if site_title != "DAGs":
+ site_title = site_title.replace("\"", "").replace("'", "")
Review comment:
Thanks both, done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490454828
##########
File path: airflow/www/templates/airflow/master.html
##########
@@ -19,6 +19,14 @@
{% extends 'appbuilder/baselayout.html' %}
+{% block page_title -%}
+ {% if title is defined -%}
+ {{ title }} - {{ app_name }}
Review comment:
@mik-laj any thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506498927
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-692970956
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669979568
> What do you think about giving the ability to change adding a unique title name for all the window?
> <img alt="Screenshot 2020-08-06 at 00 57 54" width="285" src="https://user-images.githubusercontent.com/12058428/89546233-a781ec80-d804-11ea-9782-6b24d24fff5e.png">
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669906370
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-668866450
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]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
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/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506498927
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
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 will always set identical values two after. I don't see the benefit of splitting this into two options if the user always sets the same values in the two options. We really want to get it to be able to easily recognize different environments/instances of this application.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-779269758
[The Workflow run](https://github.com/apache/airflow/actions/runs/568770817) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-749613935
@pcandoalmeida What is the status of this change? Do you need help?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466463538
##########
File path: tests/test_configuration.py
##########
@@ -658,3 +658,16 @@ def test_store_dag_code_config_when_set(self):
self.assertTrue(conf.has_option("core", "store_dag_code"))
self.assertTrue(store_serialized_dags)
self.assertFalse(store_dag_code)
+
+ @conf_vars({("webserver", "site_title"): "True"})
+ def test_site_title_config_when_set(self):
+ site_title_config = conf.get(section="webserver", key="site_title", fallback="DAGs")
+ self.assertTrue(conf.has_option("webserver", "site_title"))
+ self.assertTrue(bool(site_title_config))
+
+ @conf_vars({("webserver", "site_title"): None})
+ def test_site_title_config_when_not_set(self):
Review comment:
Hi @ashb so remove both of these tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490160211
##########
File path: airflow/www/templates/airflow/dags.html
##########
@@ -34,7 +34,7 @@
{% endblock %}
{% block content %}
- <h2>DAGs</h2>
+ <h2>{{ site_title }}</h2>
Review comment:
This one is wrong
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r466427173
##########
File path: airflow/www/views.py
##########
@@ -336,11 +336,19 @@ 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 = "Airflow - DAGs"
Review comment:
If it's only used in one place, I agree, keep our close to it's usage
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490454284
##########
File path: airflow/www/templates/airflow/master.html
##########
@@ -19,6 +19,14 @@
{% extends 'appbuilder/baselayout.html' %}
+{% block page_title -%}
+ {% if title is defined -%}
+ {{ title }} - {{ app_name }}
Review comment:
Would this be defined in `master.html` @ashb? I think this might be the more robust option, as right now I have modified several files to update the site_title variable. It works nicely as is, but the above is cleaner I feel.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pcandoalmeida commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
pcandoalmeida commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-692975873
Sure @mik-laj doing that now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#issuecomment-669568873
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb merged pull request #10162: Add Airflow UI instance_name configuration option
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #10162:
URL: https://github.com/apache/airflow/pull/10162
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r506562587
##########
File path: airflow/www/views.py
##########
@@ -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")
Review comment:
π
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r489720197
##########
File path: airflow/config_templates/config.yml
##########
@@ -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
+ version_added: ~
+ type: string
+ example: ~
+ default:
Review comment:
We could actually add the default here, so that users can easily override it.
```suggestion
default: Airflow
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #10162: Add Airflow UI site_title configuration option
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #10162:
URL: https://github.com/apache/airflow/pull/10162#discussion_r490159461
##########
File path: airflow/config_templates/config.yml
##########
@@ -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
Review comment:
This comment needs updating now -- it's all pages, isn't it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org