Update bitnami/postgresql dependency chart to latest version

[calebwoofenden]

Update from version 10.5.3 to version 12.1.2 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.

I also updated the dependency for bitnami/postgresql listed in the Chart.yaml to point at bitnami's chart repo. This means you can just change the version of the chart in Chart.yaml and then run helm dependency update and it will fetch that version for you and place the .tgz file in the charts dir and update the Chart.lock

Closes: #27353

[boring-cyborg]

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

[potiuk]

Oh yeah. Some test failures to fix :). Good that we have them. But they should be clear guidelines on what to fix :)

[potiuk]

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 :)

[calebwoofenden]

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?

[jedcunningham]

Probably better to just turn additionalProperties on instead.

[calebwoofenden]

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.

[jedcunningham]

@calebwoofenden, can we also fail the install via NOTES if folks are using the old user/password config location?

In theory no one is using this for real instances, but I'd rather be defensive here.

(Also, I realize this is a breaking change, but I think it's worthwhile given this should be a dev only scenario)

[jedcunningham]

I've also verified the tarball is the same as the one from bitnami.

[calebwoofenden]

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?

[potiuk]

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).

[calebwoofenden]

Hm, I merged main again a couple times and now the tests are passing. Could it be flaky tests?

[potiuk]

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

[potiuk]

But no - I believe they were not flakes

[potiuk]

Yep. Sounds like the problem is real.

[calebwoofenden]

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.

[calebwoofenden]

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.

[potiuk]

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 :)

[calebwoofenden]

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.

[potiuk]

I just re-run it let's see. I would have to see the tests to know where it came from.

[potiuk]

Needs fixes I am afraid.

[potiuk]

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 ?

[dnskr]

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.

[potiuk]

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 :)

[snjypl]

@calebwoofenden @potiuk i have opened a PR #29071 with fixes to the failing unittest and also upgraded the chart to lastest version 12.1.9

i dont have permission to update this PR with those changes.

[potiuk]

Closed as #29071 has been merged.