BasPH commented on code in PR #29078:
URL: https://github.com/apache/airflow/pull/29078#discussion_r1083263425
##########
docs/helm-chart/index.rst:
##########
@@ -140,3 +140,22 @@ will not start as the migrations will not be run:
This is so these CI/CD services can perform updates without issues and
preserve the immutability of Kubernetes Job manifests.
This also applies if you install the chart using ``--wait`` in your ``helm
install`` command.
+
+.. note::
+ While deploying this Helm chart with Argo, you might encounter issues with
``DB Migrations`` not running automatically on upgrade.
Review Comment:
```suggestion
While deploying this Helm chart with Argo, you might encounter issues
with database migrations not running automatically on upgrade.
```
Don't see a point in stating "DB migrations" as code-formatted text.
##########
docs/helm-chart/index.rst:
##########
@@ -140,3 +140,22 @@ will not start as the migrations will not be run:
This is so these CI/CD services can perform updates without issues and
preserve the immutability of Kubernetes Job manifests.
This also applies if you install the chart using ``--wait`` in your ``helm
install`` command.
+
+.. note::
+ While deploying this Helm chart with Argo, you might encounter issues with
``DB Migrations`` not running automatically on upgrade.
+
+To ensure DB Migrations with Argo CD, you will need to:
+
+.. code-block::yaml
+
+ migrateDatabaseJob:
+ jobAnnotations:
+ "argocd.argoproj.io/hook": Sync
+
+and, equivalent configurations in case of Flux CD
+
+.. note::
+ This will ensure that DB Migrations run when Airflow Docker image is
upgraded. This approach has a limitation that the Db Migrations will run every
time there is a ``Sync`` event in Argo. This is a trade off for automation at
the cost of some computational loss.
Review Comment:
```suggestion
This will ensure database migrations run when the Airflow Docker image
is upgraded. This approach has a limitation in that the database migrations
will run every time there is a ``Sync`` event in Argo. This is a trade-off for
automation at the cost of some computational loss.
```
##########
docs/helm-chart/index.rst:
##########
@@ -140,3 +140,22 @@ will not start as the migrations will not be run:
This is so these CI/CD services can perform updates without issues and
preserve the immutability of Kubernetes Job manifests.
This also applies if you install the chart using ``--wait`` in your ``helm
install`` command.
+
+.. note::
+ While deploying this Helm chart with Argo, you might encounter issues with
``DB Migrations`` not running automatically on upgrade.
+
+To ensure DB Migrations with Argo CD, you will need to:
+
+.. code-block::yaml
+
+ migrateDatabaseJob:
+ jobAnnotations:
+ "argocd.argoproj.io/hook": Sync
+
+and, equivalent configurations in case of Flux CD
+
+.. note::
+ This will ensure that DB Migrations run when Airflow Docker image is
upgraded. This approach has a limitation that the Db Migrations will run every
time there is a ``Sync`` event in Argo. This is a trade off for automation at
the cost of some computational loss.
+
+.. warning::
+ While deploying with Argo CD, you may encounter issues with
``(built-in)Redis secrets`` (when you use Celery or CeleryKubernetesExecutor)
not being rotated properly after Airflow pods restart. It's recommended that
you bring your own Redis. See `Celery Backend
<production-guide#celery-backend>`__
Review Comment:
```suggestion
While deploying with Argo CD, you may encounter issues with (built-in)
Redis secrets (when you use the Celery(Kubernetes)Executor) not being rotated
properly after Airflow pods restart. It's recommended that you bring your own
Redis. See `Celery Backend <production-guide#celery-backend>`__
```
Don't see the added benefit in code highlighting "(built-in)Redis secrets".
##########
docs/helm-chart/index.rst:
##########
@@ -140,3 +140,22 @@ will not start as the migrations will not be run:
This is so these CI/CD services can perform updates without issues and
preserve the immutability of Kubernetes Job manifests.
This also applies if you install the chart using ``--wait`` in your ``helm
install`` command.
+
+.. note::
+ While deploying this Helm chart with Argo, you might encounter issues with
``DB Migrations`` not running automatically on upgrade.
+
+To ensure DB Migrations with Argo CD, you will need to:
+
+.. code-block::yaml
+
+ migrateDatabaseJob:
+ jobAnnotations:
+ "argocd.argoproj.io/hook": Sync
+
+and, equivalent configurations in case of Flux CD
Review Comment:
Is the code different for Flux CD? If so, could you provide an example?
##########
docs/helm-chart/index.rst:
##########
@@ -140,3 +140,22 @@ will not start as the migrations will not be run:
This is so these CI/CD services can perform updates without issues and
preserve the immutability of Kubernetes Job manifests.
This also applies if you install the chart using ``--wait`` in your ``helm
install`` command.
+
+.. note::
+ While deploying this Helm chart with Argo, you might encounter issues with
``DB Migrations`` not running automatically on upgrade.
+
+To ensure DB Migrations with Argo CD, you will need to:
Review Comment:
```suggestion
To ensure database migrations with Argo CD, you will need to add:
```
Would simply write "DB" out fully for clarity. Also, "Migrations" is not a
name so lowercase.
##########
docs/helm-chart/production-guide.rst:
##########
@@ -295,6 +295,10 @@ By default, the chart will deploy Redis. However, you can
use any supported Cele
data:
brokerUrl: redis://redis-user:password@redis-host:6379/0
+.. note::
+ It is recommended that you bring your own Redis when using in production.
The built-in Redis instance is more suitable (and intended) for experimentation
use only by enabling you to deploy Airflow quickly.
Review Comment:
```suggestion
It is recommended that you bring your own Redis when using it in
production. The built-in Redis instance is more suitable (and intended) for
experimentation, enabling you to deploy Airflow quickly.
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
