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 2022/10/28 20:22:32 UTC
[GitHub] [airflow] calebwoofenden opened a new pull request, #27357: Update bitanmi/postgresql dependency chart to latest version
calebwoofenden opened a new pull request, #27357:
URL: https://github.com/apache/airflow/pull/27357
Update from version 10.5.3 to version 12.0.0 of the bitnami/postgresql chart. This version changes the name of the username and password variables, so I've changed those here as well. Version 10.5.3 is over a year old and a lot of new features have been added since then.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1295925796
I also rebased to latest `main` - partially to help you to see the errors better and partially to verify two improvements I've done to surface "helm test" errors better and to fix the "noise" generated by static checks (side-effect of another change I implemented recently). I hope it will be clearer now to see the errors that should be fixed @calebwoofenden :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1328341534
Needs fixes I am afraid.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1310445290
Thanks, I didn't realize that the full tests were not running unless you triggered them. I ran the Breeze setup locally and identified the issue.
The `postgres` user was being created with a randomly-generated password instead of using the value in `auth.password`. The parameters of the bitnami/postgresql chart didn't behave exactly how I thought - it turns out that `auth.username` and `auth.password` are only for accounts other than the `postgres` admin user. To set the password for the `postgres` user you need to set `auth.postgresPassword`. I added this parameter as well as `auth.enablePostgresUser`, and added more detail to the `NOTES.txt` fail message.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dnskr commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
dnskr commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1367575319
Hi @calebwoofenden!
It would be great to have your PR merged to 1.8.0 chart release. Will you have time to have a look and fix the test? Looks like it should be easy to make the last step after great effort you made.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1304987648
> Not really sure what to make of the test failures now, though I think I saw this failure on a few previous test runs (but not all).
>
> ```
> > assert sorted(list_of_sa_names_in_objects) == sorted(CUSTOM_SERVICE_ACCOUNT_NAMES)
> E AssertionError: assert ['TestCleanup...stRedis', ...] == ['TestCleanup...stRedis', ...]
> E Left contains one more item: 'default'
> E Use -v to get the full diff
> ```
>
> It seems like this is saying there is an extra ServiceAccount being created called `default`?
You can just run the test yourself. It's easy to run the tests and you should be able to debug it with pytest. You can run the pytest tests inside `breeze` development environment - https://github.com/apache/airflow/blob/main/BREEZE.rst. Should take few minutes to set-up and then once you enter Breeze you shouls be able to run `pytest tests/chart/.....` and debug and see more details on what is the problem.
Also apparently the K8S tests fail - airflow installation via the Helm Chart fails with timeout. You can a https://github.com/apache/airflow/blob/main/TESTING.rst#running-tests-with-kubernetes - there you can see even step-by-step instructions how to create cluster, deploy airflow and run tests. Likely you would need to see in the k8s logs why installing airflow via the Helm chart fails (again breeze has full support to make it super- easy including running k9s tool that helps with k8s inspection and debugging).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1008696430
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
Do I understand it correctly that we do not HAVE TO updatre to get it works (I see that the binary .tgz is replaced)
If so - this is really cool.
We had it initially done like that but that caused unnecessary dependency on the external chart which is a bad idea in some scenarios - for example when user instals the charts behind firewalls from local sources. It also introduced instability in our CI process.
So we decided to embed the .tgz (see https://github.com/apache/airflow/issues/24037)
However - if I understand correctly - this one is purely to make it easier in the future if we want to upgrade (or it makes it easy for users to upgrade if they want to them themselves? This does not introduce "necessity" of running `helm update` correct?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1306106644
But no - I believe they were not flakes
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1313946102
> Thanks, I didn't realize that the full tests were not running unless you triggered them. I ran the Breeze setup locally and identified the issue.
Only for new contributor who never merged a PR before :) . Once we merge this one, the word (Airlfow's one) is yours :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1317747986
i re-run the tests for sqlite, but seems like the Unit tests for Helm Chart need to be fixed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1336523728
I think there is one simple test to fix (and I guess we are close to release a Helm chart shortly (we just released Airflow 2.5.0 and usually Helm Chart follows shortly). Can you please fix the problem @calebwoofenden ?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1311793293
Fixed the json schema for the new params to allow null. There's still one test failure, `test_rbac.py:test_service_account_custom_names_in_objects`. It seems to be failing because the namespace includes a ServiceAccount called `default`, which it seemingly doesn't expect. As far as I can tell every namespace will contain a `default` ServiceAccount. I'm kind of at a loss on this one.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1010921247
##########
newsfragments/27353.significant.rst:
##########
@@ -0,0 +1,4 @@
+The helm chart is now using a newer version of bitnami/postgresql (from 10.5.3 to 12.0.0)
Review Comment:
```suggestion
The helm chart is now using a newer version of bitnami/postgresql (from 10.5.3 to 12.0.1)
```
##########
chart/Chart.yaml:
##########
@@ -34,7 +34,8 @@ keywords:
- scheduler
dependencies:
- name: postgresql
- version: 10.5.3
+ version: 12.0.0
Review Comment:
```suggestion
version: 12.0.1
```
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
Review Comment:
```suggestion
version: 12.0.1
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1010921066
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
Review Comment:
```suggestion
version: 12.0.1
```
This version was just released, might as well jump to it since we aren't quite ready to merge.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1297329486
> Oh yeah. Some test failures to fix :). Good that we have them. But they should be clear guidelines on what to fix :)
I see one about the newsfragment file which is an easy change. I also see this:
```
Error: templates/: values don't meet the specifications of the schema(s) in the following chart(s):
airflow:
- postgresql.auth: Additional property enablePostgresUser is not allowed
- postgresql.auth: Additional property existingSecret is not allowed
- postgresql.auth: Additional property postgresPassword is not allowed
- postgresql.auth: Additional property usePasswordFiles is not allowed
- postgresql.auth: Additional property database is not allowed
- postgresql.auth: Additional property replicationPassword is not allowed
- postgresql.auth: Additional property replicationUsername is not allowed
- postgresql.auth: Additional property secretKeys is not allowed
```
I guess because I'm including a couple of properties under `postgresql.auth` it wants me to include all of the properties that are available under that key from the bitnami chart?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1012988012
##########
chart/templates/NOTES.txt:
##########
@@ -187,3 +187,9 @@ Information on how to set a static webserver secret key can be found here:
https://airflow.apache.org/docs/helm-chart/stable/production-guide.html#webserver-secret-key
{{- end }}
+
+{{- if or .Values.postgresql.postgresqlUsername .Values.postgresql.postgresqlPassword }}
+
+ {{ fail "postgresql.postgresqlUsername and postgresql.postgresqlPassword have been renamed to postgresql.auth.username and postgres.auth.password }}
Review Comment:
Ah, good catch. Fixed the missing end quote. I just tested locally by running this command from the root of the repo:
```
helm install --debug --dry-run airflow ./chart --set-string postgresql.postgresqlPassword=blah
```
and same with `postgresql.postgresqlUsername` and it failed as expected in both situations. It succeeded without the `--set-string`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] snjypl commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by "snjypl (via GitHub)" <gi...@apache.org>.
snjypl commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1398884543
@potiuk i have opened a PR with fixes to the failing unittest and also upgraded the chart to lastest version 12.1.9
https://github.com/apache/airflow/pull/29071
i dont have permission to update this PR with those 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1295826672
Oh yeah. Some test failures to fix :). Good that we have them. But they should be clear guidelines on what to fix :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1012367795
##########
chart/templates/NOTES.txt:
##########
@@ -187,3 +187,9 @@ Information on how to set a static webserver secret key can be found here:
https://airflow.apache.org/docs/helm-chart/stable/production-guide.html#webserver-secret-key
{{- end }}
+
+{{- if or .Values.postgresql.postgresqlUsername .Values.postgresql.postgresqlPassword }}
+
+ {{ fail "postgresql.postgresqlUsername and postgresql.postgresqlPassword have been renamed to postgresql.auth.username and postgres.auth.password }}
Review Comment:
```suggestion
{{ fail "postgresql.postgresqlUsername and postgresql.postgresqlPassword have been renamed to postgresql.auth.username and postgres.auth.password" }}
```
Looks like this is causing the unit test failures. Can you test this part out manually as well? Thanks.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1385022921
> Hi @calebwoofenden! It would be great to have your PR merged to 1.8.0 chart release. Will you have time to have a look and fix the test? Looks like it should be easy to make the last step after great effort you made.
Yep. Would be great to merge that one in :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #27357: Update bitnami/postgresql dependency chart to latest version
URL: https://github.com/apache/airflow/pull/27357
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1399144100
Closed as #29071 has been merged.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1297377692
Probably better to just turn `additionalProperties` on instead.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1306145849
Yep. Sounds like the problem is real.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1008696430
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
Do I understand it correctly that we do not HAVE TO update to get it works (I see that the binary .tgz is replaced)
If so - this is really cool.
We had it initially done like that but that caused unnecessary dependency on the external chart which is a bad idea in some scenarios - for example when user instals the charts behind firewalls from local sources. It also introduced instability in our CI process.
So we decided to embed the .tgz (see https://github.com/apache/airflow/issues/24037)
However - if I understand correctly - this one is purely to make it easier in the future if we want to upgrade (or it makes it easy for users to upgrade if they want to them themselves? This does not introduce "necessity" of running `helm update` correct?
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
Do I understand it correctly that we do not HAVE TO update to get it works (I see that the binary .tgz is replaced not removed)
If so - this is really cool.
We had it initially done like that but that caused unnecessary dependency on the external chart which is a bad idea in some scenarios - for example when user instals the charts behind firewalls from local sources. It also introduced instability in our CI process.
So we decided to embed the .tgz (see https://github.com/apache/airflow/issues/24037)
However - if I understand correctly - this one is purely to make it easier in the future if we want to upgrade (or it makes it easy for users to upgrade if they want to them themselves? This does not introduce "necessity" of running `helm update` correct?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1318535845
Yes, it looks like it's the same failure I saw with this test case finding a ServiceAccount called `default` and not liking that. Every namespace must have a `default` ServiceAccount though. I don't really know where to start with this one.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1313947608
> Fixed the json schema for the new params to allow null. There's still one test failure, test_rbac.py:test_service_account_custom_names_in_objects. It seems to be failing because the namespace includes a ServiceAccount called default, which it seemingly doesn't expect. As far as I can tell every namespace will contain a default ServiceAccount. I'm kind of at a loss on this one.
Let's see the results
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1306106412
You are a new user and you have not merged any PR to Airlfow yet, so by github limitations we need to approve your workflows after you rebase/merge. I just did, so we will yet see if they fail
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1317907177
> i re-run the tests for sqlite, but seems like the Unit tests for Helm Chart need to be fixed.
Yep. one test failing.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1009478007
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
Perfect! Thanks for doing it this way :)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jedcunningham commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1301461639
I've also verified the tarball is the same as the one from bitnami.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1295428813
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/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/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/main/docs/apache-airflow/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/main/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/main/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://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1008696430
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
Do I understand it correctly that we do not HAVE TO update to get it works (I see that the binary .tgz is replaced not removed)
If so - this is really cool.
We had it initially done like that but that caused unnecessary dependency on the external chart which is a bad idea in some scenarios - for example when user instals the charts behind firewalls from local sources. It also introduced instability in our CI process.
So we decided to embed the .tgz (see https://github.com/apache/airflow/issues/24037 and PR #24395)
However - if I understand correctly - this one is purely to make it easier in the future if we want to upgrade (or it makes it easy for users to upgrade if they want to them themselves? This does not introduce "necessity" of running `helm update` correct?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1298995538
Looks like tests are passing now! The new version of the postgres chart changes the "headless" service name suffix from "headless" to "hl" and that was throwing off a bunch of tests. Thanks for the tip about the `additionalProperties` parameter also.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on a diff in pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on code in PR #27357:
URL: https://github.com/apache/airflow/pull/27357#discussion_r1009440581
##########
chart/Chart.lock:
##########
@@ -1,5 +1,6 @@
dependencies:
- name: postgresql
- version: 10.5.3
-digest: sha256:7104938113e866364a96f78802697be6bd121526cccbd06cae6a38827b7b36f9
-generated: "2022-06-11T22:19:11.389661+02:00"
+ repository: https://charts.bitnami.com/bitnami
+ version: 12.0.0
+digest: sha256:7de3ef06bed45277fef19fe8290f08d0b744c12aeb34eee9babf1e9d1a429cb2
Review Comment:
I see, thanks for the explanation. I was wondering why the whole .tgz was included, but that makes sense.
> However - if I understand correctly - this one is purely to make it easier in the future if we want to upgrade (or it makes it easy for users to upgrade if they want to them themselves? This does not introduce "necessity" of running helm update correct?
Yes, that's correct. Since the .tgz is still included, people shouldn't need to run `helm dependency update` just to install the chart. This should just mean that all it will take to update the version of the bitnami/postgresql chart used here is to update the version in the `Chart.yaml` and then run `helm dependency update` and it should pull in the new chart .tgz and update the `Chart.lock`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1302473514
Not really sure what to make of the test failures now, though I think I saw this failure on a few previous test runs (but not all).
```
> assert sorted(list_of_sa_names_in_objects) == sorted(CUSTOM_SERVICE_ACCOUNT_NAMES)
E AssertionError: assert ['TestCleanup...stRedis', ...] == ['TestCleanup...stRedis', ...]
E Left contains one more item: 'default'
E Use -v to get the full diff
```
It seems like this is saying there is an extra ServiceAccount being created called `default`?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] calebwoofenden commented on pull request #27357: Update bitanmi/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
calebwoofenden commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1305928088
Hm, I merged main again a couple times and now the tests are passing. Could it be flaky 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #27357: Update bitnami/postgresql dependency chart to latest version
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27357:
URL: https://github.com/apache/airflow/pull/27357#issuecomment-1319384895
I just re-run it let's see. I would have to see the tests to know where it came from.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org