Enforce expected tags.monitoring=false behavior #1927
Conversation
…take precedence (TIL)
|
@danielhoherd since we have enabled nodeExporter as true in https://github.com/astronomer/astronomer/blob/master/values.yaml#L75 |
Yeah, that's the source of the problem. Anything in |
|
Actually, TIL that we can completely remove the conditions, which avoids the conflict between conditions and values. This works for our use case and the way we have designed our chart. |
|
After digging a bit, it looks like our Astronomer chart is not using tags and conditions best practices, and this is not the right time to rip that out and fix it. After my last comment I was hoping to simplify the conditions, but I am not going to do so. I think this PR is good to go. CC @pgvishnuram https://helm.sh/docs/chart_best_practices/dependencies/#conditions-and-tags |
| - name: alertmanager | ||
| condition: global.alertmanagerEnabled | ||
| tags: | ||
| - monitoring | ||
| - name: prometheus | ||
| condition: global.prometheusEnabled | ||
| - name: grafana | ||
| condition: global.grafanaEnabled | ||
| tags: | ||
| - monitoring | ||
| - name: prometheus-postgres-exporter | ||
| condition: global.prometheusPostgresExporterEnabled | ||
| - name: kube-state | ||
| condition: global.kubeStateEnabled | ||
| tags: | ||
| - monitoring | ||
| - name: alertmanager | ||
| condition: global.alertmanagerEnabled | ||
| - name: prometheus-blackbox-exporter | ||
| condition: global.blackboxExporter.Enabled | ||
| tags: | ||
| - monitoring | ||
| - name: kube-state | ||
| condition: global.kubeStateEnabled | ||
| - name: prometheus-node-exporter | ||
| condition: global.nodeExporter.Enabled | ||
| tags: | ||
| - monitoring | ||
| - name: prometheus-node-exporter | ||
| condition: global.nodeExporterEnabled | ||
| - name: prometheus-postgres-exporter | ||
| condition: global.prometheusPostgresExporter.Enabled | ||
| tags: | ||
| - monitoring | ||
| - name: prometheus-blackbox-exporter | ||
| condition: global.blackboxExporterEnabled | ||
| - name: prometheus | ||
| condition: global.prometheusEnabled |
There was a problem hiding this comment.
I like the alphabetical order improvement, easier to find a dependency
Description
tags.monitoring=falsewas not disabling all components when it was expected to do so. This was caused by two different implementations of the conditions used in the tags. Components whose conditions were set totruewould always be on, and components whose conditions were not used elsewhere would work as expected. This is because helm values override the tags behavior, so if a condition is set to true in values.yaml, disabling the tag has lower precedence and is ignored.The fix I have in this branch may not be the right fix. It does what is expected, but I am not sure if it is the best way to solve the problem. It may be acceptable for our use case.
While working on debugging this I also made the following changes:
DEBUG=1show additional test data and not delete temp helm values filesRelated Issues
https://github.com/astronomer/issues/issues/5714
Testing
I have done a lot of testing on this, and most of the changes are additional tests. Some template files have been renamed though, which may cause undesirable behaviors in operation, such as a deletion of a thing followed by the addition of a thing, which could cause things to shuffle around or potentially break. This is an undesirable behavior, so we should test this and consider the stability of existing deployments compared with the value of this additional test coverage.
Merging
These changes fix a bug, so they should be merged everywhere.