SONAR-23559 Improves editions and versions setting for sonarqube chart

[davividal]

SONAR-23559

[carminevassallo]

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.

[carminevassallo]

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

[davividal]

Using schema validation and fail will lead to a fail-fast mechanism. Still, I think that fail will clutter the templates by trying to mimic something already available to us through schema validation. :)

As for simplifying the validation, when I was rebasing my PR against master (after merging yours), I (expectedly) got the validation error when running the tests. Despite reviewing your PR, it still took me a couple of minutes to understand and locate the template file. Comparing both solutions side-by-side, I don't think that replacing this with that would improve the clarity.

While I agree that the Helm's output is not ideal, I still think the overall solution is easier to understand and maintain.

WDYT?

[carminevassallo]

Thanks for clarifying. In my opinion, we should favor the readability of the error above all. While I see that in helm we don't really have standard way to fail-fast, the following two error lines that are shown in the current implementation are "internal" errors and should not be shown.

- (root): Must validate "then" as "if" was valid
- (root): Must validate all the schemas (allOf)

If we follow the "simplified validation", we cleanup the error messages displaying only what's relevant and we don't lose in debugging "power" as there is a clear execution error message pointing to the file, e.g., "Error: execution error at (sonarqube/templates/validation.yaml:2:4)". Furthermore, at the validation-failure stage, the user is not supposed to navigate internally into the chart and debug the failure. If we adopt clear failure messages, the users will be clearly notified about what's wrong and how the deployment can be fixed (e.g., which parameters are not accepted, which ones need to be now provided). My proposal would still be to use the validation through the validation.yaml file, wdyt?

[davividal]

What do you think now, @carminevassallo?

I kept the enum in the values.schema, as the error message seems clear and relevant enough to me:

$ helm template charts/sonarqube -f tests/unit-test/test-cases-values/sonarqube/invalid-edition.yaml --set monitoringPasscode=abc42
Error: values don't meet the specifications of the schema(s) in the following chart(s):
sonarqube:
- edition: edition must be one of the following: "developer", "enterprise"

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube