SONAR-23559 Improves editions and versions setting for sonarqube chart #606
Conversation
davividal
requested review from
jCOTINEAU and
carminevassallo
and removed request for
jCOTINEAU
carminevassallo
force-pushed
the
dkv/sonar-23559
branch
from
861c26e to
816a8ee
Compare
carminevassallo
force-pushed
the
dkv/sonar-23559
branch
from
816a8ee to
cfbb48e
Compare
|
There was a problem hiding this comment.
Hi @davividal,
the basic logic works, I could validate locally 🎉
However, there are a few things that I'd suggest we improve. Please let me know your opinion.
| cd "$(dirname "$0")/../tests/dynamic-compatibility-test" | ||
|
|
||
| go mod tidy | ||
| go test -timeout=0 -v schema_test.go |
There was a problem hiding this comment.
It's super cool that you added more tests! It will be then easier to spot the existence of drift from our desired tagging logic! However, I consider this as static tests, as they do not require to apply our helm chart to a cluster and run SonarQube. Can we move schema_test.go to the unit-compatibility-test folder?
| } | ||
| } | ||
| }, | ||
| "allOf": [ |
There was a problem hiding this comment.
Based on the acceptance criteria of the task, I think that adding this strong validation of the fields is not required.
Specifically, when edition is set, as of now we always favor this, regardless of whether community.enabled is set or not. The downside of adding the strong validation is to have quite a few warnings (not really meaningful) written when edition is not set.
E.g.
Error: values don't meet the specifications of the schema(s) in the following chart(s):
sonarqube:
- (root): Must validate "then" as "if" was valid
- (root): edition is required
- (root): Must validate all the schemas (allOf)
Only the second warning is really actionable.
Having said that, shall we simplify the validation? wdyt? You might also look at this PR and the usage of the fail-fast mechanism in charts/sonarqube-dce/templates/validation.yaml
SONAR-23559