*-1 *: Although the Charts work very well and smooth (great job everyone who made it!), the package does not pass the licence check. Also I have some comments particularly to the docs and potential confusion of users on how to set variables, provenience and package name.
*The formal checks:* * signature [OK] * shasum [OK] (caveat - the name in the .sha file is helm-chart/airflow-1.0.0.tgz rather than airflow-1.0.0.tgz which makes it difficult to verify automatically following the commands we have for airflow- you need to visually compare the shasum. * licenses [NOK] -> here is where the check failed We have a few of our files that are missing licences (Chart/values/schemas) but also we have the whole postgresql chart which I believe should not be there at all (I think it should be pulled from the postgres bitnami helm chart). Results of the RAT check: Files with unapproved licenses: ./Chart.yaml ./values.schema.json ./values.yaml ./values_schema.schema.json ./charts/postgresql/.helmignore ./charts/postgresql/Chart.yaml ./charts/postgresql/README.md ./charts/postgresql/values-production.yaml ./charts/postgresql/values.yaml ./charts/postgresql/files/README.md ./charts/postgresql/files/conf.d/README.md ./charts/postgresql/files/docker-entrypoint-initdb.d/README.md ./charts/postgresql/templates/NOTES.txt ./charts/postgresql/templates/_helpers.tpl ./charts/postgresql/templates/configmap.yaml ./charts/postgresql/templates/extended-config-configmap.yaml ./charts/postgresql/templates/initialization-configmap.yaml ./charts/postgresql/templates/metrics-svc.yaml ./charts/postgresql/templates/networkpolicy.yaml ./charts/postgresql/templates/secrets.yaml ./charts/postgresql/templates/serviceaccount.yaml ./charts/postgresql/templates/servicemonitor.yaml ./charts/postgresql/templates/statefulset-slaves.yaml ./charts/postgresql/templates/statefulset.yaml ./charts/postgresql/templates/svc-headless.yaml ./charts/postgresql/templates/svc-read.yaml ./charts/postgresql/templates/svc.yaml There are also some less severe problems out of which at least the doc update should be addressed before RC2 (but also docs can be updated later): *Running the chart: * I Installed the chart following the documentation: http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/helm-chart/latest/index.html in a few variants (different executors, enabling example dags). There are a few nits that I think at the very least should be corrected in the documentation (the docs might be updated after release) but possibly we might want to find an easier way to override environment variables. I tried to follow the docs when enabling local examples (which I am sure most people will do first) and found it quite a bit difficult to figure out how to do it. The documentation says that the value of the "extraEnv" parameter should be: extraEnv: "- name: AIRFLOW__CORE__LOAD_EXAMPLES\n value: True" The only way I found it works with helm command line was this however: helm upgrade airflow . --namespace airflow --set extraEnv="- name: AIRFLOW__CORE__LOAD_EXAMPLES value: \"True\"" There are three problems with it: 1) `\n` in Bash/Zsh should be the "real" EOL not \n . If you pass \n you get rather cryptic error "Error: UPGRADE FAILED: YAML parse error on airflow/templates/flower/flower-deployment.yaml: error converting YAML to JSON: yaml: line 124: mapping values are not allowed in this context" 2) if you replace \n with rea EOL, in the parameters section there are 3 spaces but should be 2 (otherwise Helm complains with another cryptic message: Error: YAML parse error on airflow/templates/flower/flower-deployment.yaml: error converting YAML to JSON: yaml: line 125: mapping values are not allowed in this context 3) the "quotes" are missing around "True" which ends up with even more cryptic error dumping few pages long yaml converted to json ending with something like ": []v1.Container: v1.Container.Env: []v1.EnvVar: v1.EnvVar.Value: ReadString: expects " or n, but found t, error found in #10 byte of ...|,"value":true},{"nam|..., bigger context ...|:[{"name":"AIRFLOW__CORE__LOAD_EXAMPLES","value":true},{"name":"AIRFLOW__CORE__FERNET_KEY","valueFro|..." I know helm/k8s fairly well so I was able to figure out quite quickly what's wrong, but this might be a blocker if documentation does not have some very clear (and correct) examples about setting the env variables. Even if you set the values via `-f myvals.yml" and people will know how to modify envVars, the case 3) above with missing quotes will be misleading. I think setting Env variables will be something people will be doing often. I think people are much more used to setting values in the charts when they are trying out the helm rather than updating the 'values.yml'. Especially when we move it to a URL-reachable helm chart. So I think at least the docs should be updated, and what's more I think maybe we should improve the way how env variables are passed? *Provenience* I think XD is right - we should add provenience - especially that the chart is going to be published at "airflow.apache.org" as far as I understand - this will then be the only way for people to verify it's correctness (there will be no package/signature). If we are releasing RC2, I would love to have it. *Name of the package* The package name has two problems: 1) airflow-1.0.0.tgz after you download it, suggest "airflow" is the content. It is of course in the "helm-chart" folder, but once you download it, it is quite ambiguous, what's inside. "airlfow-chart-1.0.0.tgz" IMHO. 2) the version does not have 1.0.0-rc1 prefix - which is a bit confusing. When we release rc2 after download we will not know which is which. J. On Sat, May 15, 2021 at 12:52 PM Xiaodong Deng <[email protected]> wrote: > Sounds good to međź‘Ť > > Will further test and get back with feedbacks. > > Thanks again Kaxil. > > > XD > > On Sat, May 15, 2021 at 12:49 Kaxil Naik <[email protected]> wrote: > >> Hi XD, yes I purposely didn't add it in favor of ASF SHA checks like we >> do for other ASF artifacts as that is mandatory. >> >> Probably for the next release we might have "both" but it wanted to get >> the first Official release out for the Helm Chart now that code-wise we are >> looking good :) and users have been waiting for a long time. >> >> Regards, >> Kaxil >> >> >> >> On Sat, May 15, 2021, 11:11 Xiaodong Deng <[email protected]> wrote: >> >>> Thanks Kaxil for running the release/voting! >>> >>> I notice we did not add the provenance file >>> <https://helm.sh/docs/topics/provenance/> for the chart. Shall we >>> consider adding it or do you find it unnecessary? >>> >>> If I missed/misunderstood any point, kindly let me know. >>> >>> Thanks! >>> >>> >>> XD >>> >>> On Sat, May 15, 2021 at 2:04 AM Kaxil Naik <[email protected]> wrote: >>> >>>> Hello Apache Airflow Community, >>>> >>>> This is a call for the vote to release the first stable version of the Helm >>>> Chart version 1.0.0 that supports both Airflow 1.10.x and Airflow 2.x. >>>> >>>> The release candidate: >>>> >>>> https://dist.apache.org/repos/dist/dev/airflow/helm-chart/airflow-1.0.0.tgz >>>> >>>> Chart Directory: >>>> https://dist.apache.org/repos/dist/dev/airflow/helm-chart/ >>>> >>>> *airflow-1.0.0.tgz* is a source release that comes with INSTALL >>>> instructions. >>>> >>>> Public keys are available at: https://www.apache.org/dist/airflow/KEYS >>>> >>>> For convenience index.yaml has been uploaded (though excluded from >>>> voting), so you can also run the below commands >>>> >>>> helm repo add apache-airflow >>>> https://dist.apache.org/repos/dist/dev/airflow/helm-chart/ >>>> helm install airflow apache-airflow/airflow >>>> >>>> The vote will be open for at least 72 hours (2021-05-18 00:05 UTC) or >>>> until the necessary number of votes are reached. >>>> >>>> >>>> https://www.timeanddate.com/countdown/to?iso=20210518T0100&p0=136&font=cursive >>>> >>>> Please vote accordingly: >>>> >>>> [ ] +1 approve >>>> [ ] +0 no opinion >>>> [ ] -1 disapprove with the reason >>>> >>>> Only votes from PMC members are binding, but members of the community >>>> are >>>> encouraged to test the release and vote with "(non-binding)". >>>> >>>> Please note that the version number excludes the `rcX` string, so it's >>>> now >>>> simply 1.0.0. This will allow us to rename the artifact without >>>> modifying >>>> the artifact checksums when we actually release. >>>> >>>> Thanks, >>>> Kaxil Naik >>>> >>> -- +48 660 796 129
