[stable/concourse] Make values.yaml more consistent

[cirocosta]

What this PR does / why we need it:

Previously, the values.yaml for stable/concourse didn't look much consistent, having some values with totally different commenting formats compared to others, making hard for us to establish a baseline of how those values should be documented and enforce them when reviewing PRs.

This commit also improves the documentation around some of those values that are not very easy to infer what they're all about.

Which issue this PR fixes

Special notes for your reviewer:

These changes do not change any behavior, and should not introduce any backward incompatibility - it's all about style and improving the documentation around the values.

However:

• there are many changes here! Please let me know if you spot any typo or a dumb change that I erroneously made; and
• garden properties are unchanged: that's on purpose as in 5.x (to be released very soon), those will all go away in favor of a garden config file (if you don't agree with not applying the changes to them, please let me know!)

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• DCO signed
• Chart Version bumped
• Variables are documented in the README.md

cc @william-tran

[cirocosta]

As a result, now the templates from this Chart are all "kubeval"-ready:

find ./templates/ -name "*.yaml" | xargs -I[] helm template . -x [] | kubeval
The document stdin contains a valid Deployment
The document stdin contains a valid Service
The document stdin contains a valid Secret
The document stdin contains a valid ClusterRole
The document stdin is empty
The document stdin contains a valid PodDisruptionBudget
The document stdin is empty
The document stdin contains a valid RoleBinding
The document stdin contains a valid Role
The document stdin contains a valid RoleBinding
The document stdin contains a valid StatefulSet
The document stdin contains a valid ServiceAccount
The document stdin is empty
The document stdin contains a valid Namespace
The document stdin contains a valid ServiceAccount
The document stdin contains a valid Service

[cirocosta]

damn, this one was me being stupid and using git add --all .. Lesson learned 👍

[cirocosta]

Hey,

As a result of this huge PR, we now have:

• all the templates declared in this chart producing kubeval validated k8s objects (see [stable/concourse] Make values.yaml more consistent #11296 (comment)); and
• all configurations being uncommented so that tools that interpret yaml (like https://github.com/cirocosta/charts-values-check) can programatically find if any values are missing documentation (which should help [stable/concourse] Include documentation for Concourse-specific values in README #11300 a lot).

If you're curious, running charts-values-check, we can identify that the chart has 325 values that can be possibly set (wow!).

---

cc @xulsitatirev @william-tran

Would you mind taking a look too @ralekseenkov ?

Thx!

[ghost]

I would split it into two separate pull requests.

• The one does changes.
• The other that documents things.
  At the moment it is super-large to review, but I will sit down and review that tomorrow.

[romangithub1024]

looks reasonable from the first glance. as long as there are no unexpected side effects related to uncommenting a bunch of values...

[cirocosta]

Oh yeah, I totally agree with you @xulsitatirev 👍 This one is just about making values.yaml parseable and having the comments consistent. The other (documenting in the README) is then up to a PR that addresses #11300

@ralekseenkov true! Thanks!

[william-tran]

Assuming empty values fail the condition {{- if .Values.concourse.web.someValue }} then I see no issues with this. It wouldn't be good if values that are defined but empty pass that condition, which would then introduce a ton of empty env vars. While the application wouldn't behave differently, kubectl describe pod would be a lot less readable.

[cirocosta]

Yeah, that's a good point, I'll give it another check at the values that I got uncommented here in this PR to make sure the diff between what gets set right now and after this PR doesn't change much.

Thx!

[cirocosta]

oh yeah, the changes here indeed introduce a bunch of extra env vars that we don't really need:

web

diff --git a/tmp/before b/tmp/after
index dbfde672..12eb94b3 100644
--- a/tmp/before
+++ b/tmp/after
@@ -1,5 +1,14 @@
 CONCOURSE_ADD_LOCAL_USER
+CONCOURSE_AUTH_DURATION
+CONCOURSE_BAGGAGECLAIM_RESPONSE_HEADER_TIMEOUT
+CONCOURSE_BIND_IP
 CONCOURSE_BIND_PORT
+CONCOURSE_BUILD_TRACKER_INTERVAL
+CONCOURSE_CONTAINER_PLACEMENT_STRATEGY
+CONCOURSE_DEBUG_BIND_IP
+CONCOURSE_DEBUG_BIND_PORT
+CONCOURSE_GLOBAL_RESOURCE_CHECK_TIMEOUT
+CONCOURSE_INTERCEPT_IDLE_TIMEOUT
 CONCOURSE_KUBERNETES_IN_CLUSTER
 CONCOURSE_KUBERNETES_NAMESPACE_PREFIX
 CONCOURSE_MAIN_TEAM_LOCAL_USER
@@ -8,8 +17,14 @@ CONCOURSE_POSTGRES_DATABASE
 CONCOURSE_POSTGRES_HOST
 CONCOURSE_POSTGRES_PASSWORD
 CONCOURSE_POSTGRES_USER
+CONCOURSE_RESOURCE_CHECKING_INTERVAL
+CONCOURSE_RESOURCE_TYPE_CHECKING_INTERVAL
 CONCOURSE_SESSION_SIGNING_KEY
 CONCOURSE_TSA_AUTHORIZED_KEYS
+CONCOURSE_TSA_BIND_DEBUG_PORT
+CONCOURSE_TSA_BIND_IP
 CONCOURSE_TSA_BIND_PORT
+CONCOURSE_TSA_HEARTBEAT_INTERVAL
 CONCOURSE_TSA_HOST_KEY
+CONCOURSE_TSA_LOG_LEVEL
 POD_IP

worker

diff --git a/tmp/before b/tmp/after
index f96f5649..3e93f5b5 100644
--- a/tmp/before
+++ b/tmp/after
@@ -1,5 +1,16 @@
+CONCOURSE_BAGGAGECLAIM_BIND_DEBUG_PORT
+CONCOURSE_BAGGAGECLAIM_BIND_IP
+CONCOURSE_BAGGAGECLAIM_BIND_PORT
+CONCOURSE_BAGGAGECLAIM_BTRFS_BIN
 CONCOURSE_BAGGAGECLAIM_DRIVER
+CONCOURSE_BAGGAGECLAIM_LOG_LEVEL
+CONCOURSE_BAGGAGECLAIM_MKFS_BIN
+CONCOURSE_BAGGAGECLAIM_REAP_INTERVAL
+CONCOURSE_BIND_DEBUG_PORT
+CONCOURSE_BIND_IP
+CONCOURSE_BIND_PORT
 CONCOURSE_GARDEN_BIND_PORT
+CONCOURSE_LOG_LEVEL
 CONCOURSE_TSA_HOST
 CONCOURSE_TSA_PUBLIC_KEY
 CONCOURSE_TSA_WORKER_PRIVATE_KEY

I'll follow up with some changes to get that number of env vars back to the original.

[william-tran]

Maybe a helper function could help with that?

[ghost]

@william-tran What do you think about, Will?

[cirocosta]

Maybe a helper function could help with that?

Sorry, but In terms of what? I didn't understand 🤔

---

I just pushed a final squashed commit with all of those default values that got introduced removed so that concourse can use what it thinks good default values are and we don't end with a bunch of env vars that are just passing the default ones.

Now the diff shows .. no differences 😁

[ghost]

@cirocosta Approved.

[cirocosta]

Thanks for reviewing it @xulsitatirev !

cc @william-tran
cc @cpanato

[cirocosta]

/test all

[cirocosta]

cc @william-tran

[paulczar]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cirocosta, paulczar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/concourse/OWNERS [cirocosta,paulczar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment