remove null env lists

[davejrt]

Also ensures each container record starts with the container name

Fixes https://github.com/sourcegraph/sourcegraph/issues/11754

Note: Open to suggestions on how we can avoid the number of merge conflicts this might cause for customers.

[uwedeportivo]

the only reason i can think of that the empty env keys are there is that we wanted to document in the YAML the place where customers should put env vars.

one way to try to avoid complicated merge conflicts would be to not delete the empty env vars and instead declare some DUMMY env var there. not sure if this is aesthetically acceptable or the merge is easier then. i actually wouldn't vote for it.

[pecigonzalo]

It would be lovely to have some tool that does/validates this sorting

[davejrt]

Yeah, I very much dislike that the stop of each container record isn't the name of the container when I browsing these files. Yaml is bad enough to read but then the title is like...some random arguments or env var it makes it worse.

But I see your point.

[daxmc99]

Kube-linter is a linter that comes to mind here

[davejrt]

kubelinter is nice, but it doesn't seem to specify the order of fields in lists like we do here or if a record as a null value.

running it over master results in this

dave@dave-surface-2:~/sourcegraph/deploy-sourcegraph (master)$ kube-linter lint base --exclude run-as-non-root,no-read-only-root-fs
base/backend.Service.yaml: (object: <no namespace>/backend /v1, Kind=Service) no pods found matching service labels (map[group:backend]) (check: dangling-service, remediation: Make sure your service's selector correctly matches the labels on one of your deployments.)

base/minio/minio.Deployment.yaml: (object: <no namespace>/minio apps/v1, Kind=Deployment) environment variable MINIO_SECRET_KEY in container "minio" found (check: env-var-secret, remediation: Don't use raw secrets in an environment variable. Instead, either mount the secret as a file or use a secretKeyRef. See https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets for more details.)

Error: found 2 lint errors
dave@dave-surface-2:~/sourcegraph/deploy-sourcegraph (master)$

[davejrt]

How comfortable are you with me merging this @daxmc99 @pecigonzalo (ignoring the renovate merge conflicts right now) ? Are we happy to accept the hiccup for those customers not using kustomize here to upgrade to 3.25? Perhaps if anything it's an extra motivation for them to move in that direction?

[pecigonzalo]

@davejrt I would go for it, and announce it in our changelog/release blog (no idea how to coordinate that sorry).

A bit out of scope, but as we are doing this, we might as well just sort all fields as well. Although maybe we can do that as a patch-release so its easier to diff/apply? Not sure, just throwing it out here, Im happy to merge as is to fix this issue and I much prefer the container name being the first item.

[pecigonzalo]

Oh and let CE/CSE know if we do go ahead so they know its coming after release.

[daxmc99]

This change needs to be made eventually. I'm not sure if we should sort the keys in this same PR but the ds-to-dhall tool could provide that functionality.
Could you link a draft PR of the changelog update here as well so that we can coordinate for the release?

[davejrt]

@daxmc99 changlog draft added merge conflicts sorted out. I did a trial with ds-to-dhall but we do lose the fact that name is always the top of the list...which frankly is a feature I really like. Perhaps there's something we can do later but I think for now this satisfies the immediate concern with this issue.