Added logic to verify the generated prometheus rule file #18
Conversation
Signed-off-by: Shubhendu <[email protected]>
|
@umangachapagain @cloudbehl please review |
|
@anmolsachan review please |
| @@ -29,3 +29,4 @@ script: | |||
| done | |||
|
|
|||
| - make all | |||
| - make generate_k8s | |||
There was a problem hiding this comment.
Can we give a name like kubernetes_object_yaml?
| @@ -19,3 +19,12 @@ lint: prometheus_alert_rules.yaml | |||
|
|
|||
| clean: | |||
| rm -rf prometheus_alert_rules.yaml | |||
|
|
|||
| generate_k8s: | |||
| cd extras; cp manifests/prometheus-rules.yaml .; jb install; ./build.sh example.jsonnet; cd - | |||
There was a problem hiding this comment.
instead of semicolon(;) it's better we use && syntax.
There was a problem hiding this comment.
I understand both have different purpose. With && if previous one is successful then only second would be executed. Anyway let me try that.
| cd extras; cp manifests/prometheus-rules.yaml .; jb install; ./build.sh example.jsonnet; cd - | ||
| cmp -s extras/prometheus-rules.yaml extras/manifests/prometheus-rules.yaml; \ | ||
| RETVAL=$$?; \ | ||
| if [ $$RETVAL -eq 0 ]; then \ |
There was a problem hiding this comment.
Should be "not equal to 0" ??
There was a problem hiding this comment.
Yes, that's a mistake. Will correct that.
| @@ -19,3 +19,12 @@ lint: prometheus_alert_rules.yaml | |||
|
|
|||
| clean: | |||
| rm -rf prometheus_alert_rules.yaml | |||
|
|
|||
| generate_k8s: | |||
| cd extras; cp manifests/prometheus-rules.yaml .; jb install; ./build.sh example.jsonnet; cd - | |||
There was a problem hiding this comment.
I think line 25 will always fail because jb install; will always clone the upstream. The genterated manifests/prometheus-rules.yaml from ./build.sh example.jsonnet; will always be made from the upstream cloned code. Because of this comparision will always fail.
I think before ./build.sh example.jsonnet; the following line should be added so that the cloned code gets the changes made in the pr : cp -R ./../alerts/* ./vendor/ceph-mixins/alerts/
There was a problem hiding this comment.
Yes while verifying the changes I figured this out. Will test ad send the updated PR soon.
There was a problem hiding this comment.
Exactly: And this should copy all the files that the dev can change. Eg config files, alert rule files, etc.
There was a problem hiding this comment.
This PR we will take up later as there are other issues while copying the files under vendor directory like cyclic dependency.
Signed-off-by: Shubhendu [email protected]