Hi all, I am cancelling this vote and have consulted ASF and will have an rc2 to vote on "sources" instead of binary generated by Helm.
- "helm package" removed comments (and hence License) from Chart.yml file. - helm 3.0.0 had a bug that stripped comments and hence License from values.yml file: (Issue Link <https://github.com/helm/helm/issues/6951>) - Docs will be continued to be developed separately, the "extraEnv" bug is just a docs error and fixed in https://github.com/apache/airflow/pull/15878 - Since I am going to create rc2, will add a provenance file too - "version"'s should not contain an "-rc" prefix to allow moving and voted on artifact to "release" folder in ASF: https://dist.apache.org/repos/dist/release/airflow/ otherwise we need to modify to update version which defeats the purpose of voting. - Users can directly do a "helm install URL_TO_CHART" or "helm repo add .." etc to install chart, hence just "airflow" is a chart. This is also how all the ASF & other Helm projects are doing. RC2 mail in a bit :) Regards, Kaxil On Sat, May 15, 2021 at 8:06 PM Jarek Potiuk <[email protected]> wrote: > *-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 >
