Extract Baremetal Operator CRDs into dedicated package #64
Conversation
| description: Beremetal Operator CRDs | ||
| name: baremetal-operator-crds | ||
| type: application | ||
| version: 0.3.0 |
There was a problem hiding this comment.
Do you think we should perhaps version this to align with the baremetal-operator release? e.g currently the image used is 0.4.0 which is derived from the upstream 0.4.0 release
This has the advantage of making it very clear where the CRDs come from, but the potential disadvantage is we might end up copying the CRDs chart without any changes if there are BMO releases which don't modify any CRDs.
There was a problem hiding this comment.
Isn't the appVersion the one we want to modify here? IIRC version holds only a temporary value distinguishing our (pre-) alpha versions and will be changed to 3.0.0 for the Edge release?
There was a problem hiding this comment.
Makes sense
/cc @hardys WDYT?
There was a problem hiding this comment.
Happy to be corrected, but IIUC version is the chart version used for management of dependencies, and appVersion is optional and indicates the version of whatever thing you're installing (e.g it's redundant in the case where the chart version can be exactly coupled to the deployed artefact, which in this case it can?)
It raises the question of where appVersion: 1.16.0 came from though - AFAICS that version doesn't relate to anything and could probably be removed?
There was a problem hiding this comment.
FWIW I've been looking at https://helm.sh/docs/topics/charts/#the-appversion-field and this description of simple versioning best-practice (see "Simple 1-1 versioning")
There was a problem hiding this comment.
You're correct on both version and appVersion definitions. I meant that we can use the appVersion to indicate that the CRDs are coming from the 0.4.0 version Metal3.
Some projects choose to match the version of the chart (version) to the version of the application (appVersion). However, in our case, version will later be set to 3.0.0 anyway (with an appropriate suffix if backports are necessary) as per the internal documentation we were discussing few months ago.
Side note - appVersion: 1.16.0 is the default one when a chart is generated via Helm.
There was a problem hiding this comment.
Thanks for clarifying @atanasdinov and thinking about it, there's one scenario where the simple versioning doesn't work - if we had to fix an issue in the chart (unrelated to a new BMO release) we'd want to bump version but not appVersion
So, if I'm understanding correctly that means the new chart should be e.g version: 0.1.0 and appVersion: 0.4.0 @Kristian-ZH what are your thoughts?
There was a problem hiding this comment.
if we had to fix an issue in the chart (unrelated to a new BMO release) we'd want to bump version but not appVersion
This is exactly the approach I've went with for the KubeVirt chart. version: 0.1.0 is the one which Sylva identified not suitable for their environments and I "froze" it and made all improvements (>5 separate PRs) in version: 0.2.0 instead.
There was a problem hiding this comment.
I think that making the appVersion to 0.4.0 sounds reasonable. In this way, we will use the appVersion to track versions from the upstream. For the version field I am not sure what would fit best here. For now I think that it is ok to iterate over it for our chart modification because this is also the version that is used from chart-build-script to crate the chart versioning. So making it 0.1.0 for now is probably OK. We can continue with that strategy and to update the versions only on the dependencies that are changed, and stop updating all as we currently do (all are with versions 0.4.0). Probably after each dependency version update, we will have to update the parent chart version as well.
Kristian-ZH
force-pushed
the
extract-baremetal-operator-crds
branch
2 times, most recently
from
9652a5b
to
77f23ce
Compare
| @@ -0,0 +1,6 @@ | |||
| apiVersion: v2 | |||
There was a problem hiding this comment.
Could you please explain why the chart is in both packages/baremetal-operator-crds and packages/baremetal-operator/charts/charts/baremetal-operator-crds?
This isn't the case for MetalLB for example.
There was a problem hiding this comment.
Honestly IDK...
This is the result from make prepare.
The diffs between both components are that:
- Metallb is released chart until BarametalOperator not because it is part of the
Metal3chart. - MetalLB is a root chart, until BaremetalOperator is a subchart (We have Metal3 -> BaremetalOperator -> BaremetalOperatorCRDs)
There was a problem hiding this comment.
I think you need to run make clean to get rid of this "working copy" according to the development docs.
Kristian-ZH
force-pushed
the
extract-baremetal-operator-crds
branch
from
77f23ce
to
32b1855
Compare
|
As we still have no strong opinion what is the best way to keep the CRDs, I will close this PR for now and the topic will be discussed again after the holidays |
Extract Baremetal Operator CRDs into a dedicated package.
In this way, we will have a manual version control of both packages
Closes #55