-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Terratests for operate #176
Conversation
6bbabdf
to
298ab26
Compare
Sorry, I had little time today, I'll try to review it tonight, but I'll definitely review it tomorrow at the latest 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Great stuff! No blockers from my part. This seems like a lot, but I think that's just because we accumulated so many features over time with no tests 😄 I can imagine some tests being more or less the same across all charts (e.g. the default values for templates), and maybe we can extract some common functionality once we've implemented these a few times to simplify our lives.
Please look at the questions/suggestions before merging. I don't think we need another review cycle unless you want one 👍
|
||
// then | ||
s.Require().NoError(err, "Golden file doesn't exist or was not readable") | ||
s.Require().Equal(string(expected), output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ What's the output when they aren't equal? I think the testify library comes with a JSONEq
method, maybe this provides more specialized output/comparison? It also has a YAMLEq
as well, by the way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks good to know! I check it and actually I like the failure message better than the yamleq
String eq
Error Trace: configmap_test.go:57
Error: Not equal:
expected: "---\n# Source: ccsm-helm/charts/operate/templates/configmap.yaml\nkind: ConfigMap\nmetadata:\n name: ccsm-helm-test-operate\n labels:\n app: camunda-cloud-self-managed\n app.kubernetes.io/name: operate\n helm.sh/chart: operate-0.0.14\n app.kubernetes.io/instance: ccsm-helm-test\n app.kubernetes.io/managed-by: Helm\n app.kubernetes.io/version: 1.3.1\n app.kubernetes.io/part-of: camunda-cloud-self-managed\n app.kubernetes.io/component: operate\napiVersion: v1\ndata:\n application.yml: |\n # Operate configuration file\n camunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\n logging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n #Spring Boot Actuator endpoints to be exposed\n management.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"
actual : "---\n# Source: ccsm-helm/charts/operate/templates/configmap.yaml\nkind: ConfigMap\nmetadata:\n name: ccsm-helm-test-operate\n labels:\n app: camunda-cloud-self-managed\n app.kubernetes.io/name: operate\n helm.sh/chart: operate-0.0.14\n app.kubernetes.io/instance: ccsm-helm-test\n app.kubernetes.io/managed-by: Helm\n app.kubernetes.io/version: 1.3.1\n app.kubernetes.io/part-of: camunda-cloud-self-managed\n app.kubernetes.io/component: operate\napiVersion: v1\ndata:\n application2.yml: |\n # Operate configuration file\n camunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\n logging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n #Spring Boot Actuator endpoints to be exposed\n management.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"
Diff:
--- Expected
+++ Actual
@@ -16,3 +16,3 @@
data:
- application.yml: |
+ application2.yml: |
# Operate configuration file
Test: TestConfigmapTemplate/TestContainerGoldenTestConfigMapDefaults
YamlEq:
Error Trace: configmap_test.go:56
Error: Not equal:
expected: map[string]interface {}{"apiVersion":"v1", "data":map[string]interface {}{"application.yml":"# Operate configuration file\ncamunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\nlogging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n#Spring Boot Actuator endpoints to be exposed\nmanagement.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"labels":map[string]interface {}{"app":"camunda-cloud-self-managed", "app.kubernetes.io/component":"operate", "app.kubernetes.io/instance":"ccsm-helm-test", "app.kubernetes.io/managed-by":"Helm", "app.kubernetes.io/name":"operate", "app.kubernetes.io/part-of":"camunda-cloud-self-managed", "app.kubernetes.io/version":"1.3.1", "helm.sh/chart":"operate-0.0.14"}, "name":"ccsm-helm-test-operate"}}
actual : map[string]interface {}{"apiVersion":"v1", "data":map[string]interface {}{"application2.yml":"# Operate configuration file\ncamunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\nlogging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n#Spring Boot Actuator endpoints to be exposed\nmanagement.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"}, "kind":"ConfigMap", "metadata":map[string]interface {}{"labels":map[string]interface {}{"app":"camunda-cloud-self-managed", "app.kubernetes.io/component":"operate", "app.kubernetes.io/instance":"ccsm-helm-test", "app.kubernetes.io/managed-by":"Helm", "app.kubernetes.io/name":"operate", "app.kubernetes.io/part-of":"camunda-cloud-self-managed", "app.kubernetes.io/version":"1.3.1", "helm.sh/chart":"operate-0.0.14"}, "name":"ccsm-helm-test-operate"}}
Diff:
--- Expected
+++ Actual
@@ -3,3 +3,3 @@
(string) (len=4) "data": (map[string]interface {}) (len=1) {
- (string) (len=15) "application.yml": (string) (len=823) "# Operate configuration file\ncamunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\nlogging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n#Spring Boot Actuator endpoints to be exposed\nmanagement.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"
+ (string) (len=16) "application2.yml": (string) (len=823) "# Operate configuration file\ncamunda.operate:\n # ELS instance to store Operate data\n elasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Zeebe instance\n zeebe:\n # Broker contact point\n brokerContactPoint: \"ccsm-helm-test-zeebe-gateway:26500\"\n # ELS instance to export Zeebe data to\n zeebeElasticsearch:\n # Cluster name\n clusterName: elasticsearch\n # Host\n host: elasticsearch-master\n # Transport port\n port: 9200\n # Index prefix, configured in Zeebe Elasticsearch exporter\n prefix: zeebe-record\nlogging:\n level:\n ROOT: INFO\n org.camunda.operate: DEBUG\n#Spring Boot Actuator endpoints to be exposed\nmanagement.endpoints.web.exposure.include: health,info,conditions,configprops,prometheus"
},
Test: TestConfigmapTemplate/TestContainerGoldenTestConfigMapDefaults
--- FAIL: TestConfigmapTemplate/TestContainerGoldenTestConfigMapDefaults (0.17s)
It is even more problematic when something in application.yaml changed. I will keep string eq for now, lets see maybe we have to change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more specialized assertions suggestions. The docs has a list of all available functions, and they tend to give more specialized error messages 👍 https://pkg.go.dev/github.com/stretchr/testify/require#pkg-functions
// then | ||
expectedContainerImage := "camunda/operate:a.b.c" | ||
containers := deployment.Spec.Template.Spec.Containers | ||
s.Require().Equal(1, len(containers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.Require().Equal(1, len(containers)) | |
s.Require().Len(containers, 1) |
🔧 Can be applied to others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm with the s.Require().Equal(1, len(containers))
I get:
deployment_test.go:79:
Error Trace: deployment_test.go:79
Error: Not equal:
expected: 1
actual : 2
Test: TestDeploymentTemplate/TestContainerOverwriteGlobalImageTag
--- FAIL: TestDeploymentTemplate/TestContainerOverwriteGlobalImageTag (0.21s)
With s.Require().Len(containers, 1)
I get:
deployment_test.go:57:
Error Trace: deployment_test.go:57
Error: "[{operate camunda/operate:a.b.c [] [] [{http %!s(int32=0) %!s(int32=8080) TCP }] [] [] {map[cpu:{{%!s(int64=1000) %!s(resource.Scale=-3)} {%!s(*inf.Dec=<nil>)} DecimalSI} memory:{{%!s(int64=2147483648) %!s(resource.Scale=0)} {%!s(*inf.Dec=<nil>)} 2Gi BinarySI}] map[cpu:{{%!s(int64=500) %!s(resource.Scale=-3)} {%!s(*inf.Dec=<nil>)} 500m DecimalSI} memory:{{%!s(int64=1073741824) %!s(resource.Scale=0)} {%!s(*inf.Dec=<nil>)} 1Gi BinarySI}]} [{config %!s(bool=false) /usr/local/operate/config/application.yml application.yml %!s(*v1.MountPropagationMode=<nil>) }] [] nil nil nil nil nil %!s(bool=false) %!s(bool=false) %!s(bool=false)} {lol [] [] [] [] [] {map[] map[]} [] [] nil nil nil nil nil %!s(bool=false) %!s(bool=false) %!s(bool=false)}]" should have 1 item(s), but has 2
Test: TestDeploymentTemplate/TestContainerOverwriteImageTag
--- FAIL: TestDeploymentTemplate/TestContainerOverwriteImageTag (0.16s)
I can add messages but 🤷
Messages: expect one container
--- FAIL: TestDeploymentTemplate/TestContainerOverwriteImageTag (0.22s)
One last comment: do we need to add licensing information to this? The repo is licensed, which I guess applies to the Helm chart? I don't know how the Go code fits in there. I know it's all test code, but I'm unsure about the legality here. I think we would need to add licensing to the code, in which case it can just be Apache 2. Speaking of licensing, is the CLA enabled on this repo? As for future contributions we'd probably want to have it. |
Add Makefile to simplify running go tests and development of helm charts
Use require no error function instead of conditional branches
Test should ensure that image tags are set correctly. Direct setting of operate.image.tag has higher presence than global setting.
First parameter is expected and second actual value.
First parameter is expected and second actual value.
Default type should be array instead of a map otherwise the templating doesn't make sense.
ff5ebd1
to
5b8012c
Compare
Thank so much for your time and review @npepinpe 🙇
I agree. I probably create a parameterized tests or something at some point for the default values.
CLA bot is enabled yes. For the licensing headers I created #185 |
First part of adding new helm tests.
This PR adds:
This is the first part of many. I started with operate to show it on a less complex chart and only included two template tests, to not overwhelm the reviewer.
related #125
P.S: ~1.4 lines are go sum