[FHIR] Repository CI and minor chart-specific suggestions

chgl commented 3 years ago

Thank you for this useful chart! I have a few suggestions which I would be happy to contribute if deemed appropriate. Some are possibly too opinionated/nitpicky and stem from experiences with my own chart repo over at https://github.com/chgl/charts - so feel free to disapprove of them!

Infrastructure/CI

[x] #18
[x] Add a workflow for basic chart linting using the chart-testing tool https://github.com/helm/chart-testing/blob/main/doc/ct_lint.md #36
[ ] Add a workflow for advanced chart linting using one or more of the many Kubernetes static analysis tools (E.g. I maintain a single container image with some of these tools over at https://github.com/chgl/kube-powertools. And a small script for helm chart repos running several linters: https://github.com/chgl/kube-powertools/blob/master/scripts/chart-powerlint.sh). The biggest thing all of these linters complain about is enforcing a restrictive securityContext. I'd suggest https://github.com/FairwindsOps/polaris for it's nice UI or https://github.com/stackrox/kube-linter to start or just manually run against the chart.
[x] Add a workflow for installing and basic testing of the chart in a CI-Cluster https://github.com/helm/chart-testing/blob/main/doc/ct_install.md (e.g. https://github.com/chgl/charts/blob/master/.github/workflows/ci.yaml#L40-L75) #36

Chart-specific suggestions

[x] #17
[x] #27
[x] make the securityContext of each pod and container configurable and set maximally-restrictive default values.
[x] Breaking change: Use the newer app.kubernetes.io/name etc. labels instead of the "older" app: ones (https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels)
[x] use Chart.appVersion to set the container image path to avoid having to maintain an "app version" in two places. See https://github.com/argoproj/argo-helm/blob/master/charts/argo-workflows/templates/server/server-deployment.yaml#L34 for an example
[x] use the Bitnami helm chart conventions for specifying the image's registry/repository/image to make it easier to configure a pull-through cache for Dockerhub or host the image in a custom registry:
```
  image:
    registry: docker.io
    repository: ibmcom/ibm-fhir-server
    tag: "4.9.0"
```
(https://github.com/bitnami/charts/blob/master/bitnami/airflow/values.yaml#L204)
[x] default to a replicaCount of 1 #36
[x] keep resource requests/limits unset by default but include suggestions in a comment to leave them as a conscious decision for the chart user
[x] The repository could eventually be added to ArtifactHub (https://artifacthub.io/docs/topics/repositories/#helm-charts-repositories) which makes discoverability easier and even provides the ability to create and display a nice changelog: https://artifacthub.io/packages/helm/artifact-hub/artifact-hub?modal=changelog see https://artifacthub.io/docs/topics/annotations/helm/

Nit-Picky

[x] Consider moving the chart folders inside a charts-dir because it's the default location for some helm tools
[ ] ~^{^{I think naming the repository charts would be a tad more conventional, helm repo add https://alvearie.github.io/charts also looks a bit cooler/less duplicate than https://alvearie.github.io/alvearie-helm}}~

lmsurpre commented 3 years ago

Thanks @chgl for the good suggestions. I'm thinking of turning each of these (or at least many of them) into their own issues so they can be independently discussed. Hooking up chart-releaser was definitely my plan...I had a play with it from my own repo the last time you mentioned it (over at IBM/FHIR) and I agree it will be a nice no-cost/low-hassle way of handling chart releases. Linting sounds like a good idea too, although I don't have much experience with that. I think it would be a place where we'd invite contribution. Hooking up a CI also sounds smart. My plan to date has been to leave the FHIR server testing to IBM/FHIR and to leave testing of the charts to Alverie/health-patterns...however I hadn't yet thought through how we might prevent chart errors BEFORE we pick them up in that health-patterns repo.

Consider moving the chart folders inside a charts-dir because it's the default location for some helm tools

I think I've seen that convention elsewhere but I never understood the extra level. Does it buy us anything?

I think naming the repository charts would be a tad more conventional, helm repo add https://alvearie.github.io/charts also looks a bit cooler/less duplicate than https://alvearie.github.io/alvearie-helm

I did float that alternative name past the Alvearie core team but we ended up on alvearie-helm. We didn't debate it for long, but I think the main points from that discussion were:

forks/clones will lose the org context
charts has even more overloaded meaning in healthcare
a little redundancy doesn't seem too bad

chgl commented 3 years ago

Thank you for the feedback!

Linting sounds like a good idea too, although I don't have much experience with that. I think it would be a place where we'd invite contribution.

Great! I'm a big fan of linting so I would be happy to contribute something. I think a good step would be to implement

Include postgres as an optional sub-chart to make it easier to install the server

first, which simplifies setting up the chart-testing tool (which includes a basic linting step). I can take a stab at it if it's OK.

My plan to date has been to leave the FHIR server testing to IBM/FHIR and to leave testing of the charts to Alverie/health-patterns...however I hadn't yet thought through how we might prevent chart errors BEFORE we pick them up in that health-patterns repo.

I agree that any functional testing should be kept closer to the application code. chart-testing/helm test at least in my experience ist mostly suited to just make sure the application installed via the chart starts up correctly - so a basic curl /Patient?_summary=count to make sure everything is reachable may be a "good enough".

I think I've seen that convention elsewhere but I never understood the extra level. Does it buy us anything?

The biggest advantage is definitely not having to configure any tools which assume "/charts" is the default. Especially useful if you add additional folders that aren't actually charts (like /docs, /scripts, whatever) to the root dir (although some tools additionally check for a Charts.yaml to make sure). I think of it as similar to a /src folder vs placing source files in the root directory.

I did float that alternative name past the Alvearie core team but we ended up on alvearie-helm.

those are actually great arguments - especially the first one, since I am already at charts-2 in one of my forks (https://github.com/chgl/charts-2)...

lmsurpre commented 3 years ago

I think a good step would be to implement [Include postgres as an optional sub-chart to make it easier to install the server] I can take a stab at it if it's OK.

Sounds good. I converted that one into https://github.com/Alvearie/alvearie-helm/issues/27

lmsurpre commented 3 years ago

The repository could eventually be added to ArtifactHub

It's alive! https://artifacthub.io/packages/helm/alvearie/ibm-fhir-server

Alvearie / alvearie-helm