CLOUDP-227413: AKO templates #348
Conversation
igor-karpukhin
requested review from
tibulca,
mircea-cosbuc,
helderjs,
lsierant,
slaskawi,
josvazg,
roothorp and
nammn
as code owners
|
Is there any way helm supports some form of unit testing? Was this tested manually? |
https://helm.sh/docs/topics/chart_tests/ maybe? but that is not a unit test per se. Other than that, when working manually you can tell helm to spit out the YAMl without applying with |
Yes, there is a way to add basic "template generating" tests and compare the result to the expected values. Our charts do not have tests for now. So I tested both manually. |
I think this is worth a ticket and something we'd want to tackle as part of product quality maturity, would you mind creating one? @igor-karpukhin Besides: couldn't we add an integration test in AKO at least, similar to what we do now? This would require this PR to merge first. |
There was a problem hiding this comment.
Do not have extra comments on top of @s-urbaniak
The code and the expanded default templates look good.
Have these been tested in Cloud QA?
Yes, as I said, both charts were tested on cloudqa |
|
|
||
| keywords: | ||
| - mongodb | ||
| - database | ||
| - nosql | ||
| home: https://github.com/mongodb/mongodb-atlas-kubernetes | ||
| icon: https://webimages.mongodb.com/_com_assets/cms/kuyjf3vea2hg34taa-horizontal_default_slate_blue.svg | ||
| appVersion: "1.0.0" | ||
| appVersion: "2.4.1" |
There was a problem hiding this comment.
i think my question still applies: how do we make sure that this does not introduce skew when we move forward with new AKO versions?
There was a problem hiding this comment.
In general, the helm chart version does not need to match the version of the image.
These examples are even less tied to the Operator releases, they should just keep working after most releases, unless some breaking changes are released.
I see two ways about this:
- Every Operator and Helm release requires a release in these example charts.
- These example charts are exercised in a test in the CI and will only get updated as needed, when actual changes makes them somewhat obsolete.
If we do not automate on auto testing these, we will be forced to option 1, at least until we make such investment.
There was a problem hiding this comment.
I changed it to the latest AKO version just in case. As we do the same thing for the AKO chart and the AKO CRDs
There was a problem hiding this comment.
With respect to the appVersion field: https://helm.sh/docs/topics/charts/#the-appversion-field
Note that the
appVersionfield is not related to the version field. It is a way of specifying the version of the application. For example, the drupal chart may have an appVersion: "8.2.1", indicating that the version of Drupal included in the chart (by default) is 8.2.1. This field is informational, and has no impact on chart version calculations.
With respect to the version field: https://helm.sh/docs/topics/charts/#charts-and-versioning
Every chart must have a version number. A version must follow the SemVer 2 standard. Unlike Helm Classic, Helm v2 and later uses version numbers as release markers. Packages in repositories are identified by name plus version.
My conclusions from the above are the following:
-
Given
appVersionis purely informational I vote for making it clear (as a comment) that this is the version of AKO this helm was created with. -
Given
versionis a semver version of this chart specifically I vote to set it to "1.0" since this is the very first version of this chart
@josvazg @igor-karpukhin thoughts?
There was a problem hiding this comment.
Sounds reasonable to me.
Also I would avoid having to re-release this with all released of AKO. Releasing only when new versions might force it due to breaking changes or features than need to be showcased.
All Submissions:
Added two new helm charts showcasing basic and advanced configurations
closes #XXXXin your comment to auto-close the issue that your PR fixes (if such).