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 2021/01/10 11:08:51 UTC

[GitHub] [airflow] XD-DENG opened a new pull request #13596: Have proper default for webserver.expose_config in Helm Chart

XD-DENG opened a new pull request #13596:
URL: https://github.com/apache/airflow/pull/13596


   Configuration `webserver.expose_config` should have consistent default value with the normal Airflow default settings (False). So we can simply remove it in `values.yaml`, so default value in `default_airflow.cfg` is respected.
   
   This helps encourage safer setting up in Helm context, also avoid accidental exposure of config (if users don't carefully check the config before install the chart).
   
   Ref: Slack discussion https://apache-airflow.slack.com/archives/CCPRP7943/p1610213808438200
   
   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   


----------------------------------------------------------------
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 merged pull request #13596: Have proper default for webserver.expose_config in Helm Chart

Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13596:
URL: https://github.com/apache/airflow/pull/13596


   


----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #13596: Have proper default for webserver.expose_config in Helm Chart

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13596:
URL: https://github.com/apache/airflow/pull/13596#discussion_r554551080



##########
File path: chart/values.yaml
##########
@@ -705,7 +705,6 @@ config:
     statsd_host: '{{ printf "%s-statsd" .Release.Name }}'
   webserver:
     enable_proxy_fix: 'True'
-    expose_config: 'True'

Review comment:
       I simply remove this line, so the default value in `default_airflow.cfg` is 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] XD-DENG commented on a change in pull request #13596: Have proper default for webserver.expose_config in Helm Chart

Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #13596:
URL: https://github.com/apache/airflow/pull/13596#discussion_r554551000



##########
File path: chart/README.md
##########
@@ -134,7 +134,7 @@ helm upgrade airflow . \
 
 ## Mounting DAGS using Git-Sync side car without Persistence
 
-This option will use an always running Git-Sync side car on every scheduler,webserver and worker pods. The Git-Sync side car containers will sync DAGs from a git repository every configured number of seconds. If you are using the KubernetesExecutor, Git-sync will run as an initContainer on your worker pods.
+This option will use an always running Git-Sync side car on every scheduler, webserver and worker pods. The Git-Sync side car containers will sync DAGs from a git repository every configured number of seconds. If you are using the KubernetesExecutor, Git-sync will run as an initContainer on your worker pods.

Review comment:
       This is a simple typo fix. Not related to the scope of this PR. Not worth creating a separate 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