search

suse-edge / charts

SUSE Edge engineering Helm charts
https://suse-edge.github.io/charts/
Apache License 2.0
5 stars 18 forks source link

add supported nic for the use case pw #107

Closed alknopfler closed 5 months ago

alknopfler commented 5 months ago
manuelbuil commented 5 months ago

The changes look perfect @alknopfler but I have just noticed one difference with the other repos where we have charts. In other repos, we use the variable packageVersion in packages/sriov-operator/package.yaml file, which allows us to build different charts even if they are pointing at the same upstream version for cases like what this PR is trying to fix. For example, look at calico v3.27.0: https://github.com/rancher/rke2-charts/tree/main/charts/rke2-calico/rke2-calico, we have three different versions:

Without any logic like that, if the upstream version is not changed, we would be overwriting the same chart/asset which could make it very hard to track/reproduce customer issues. Perhaps this was already discussed and it is something you'd like to address in the future? Or maybe you rejected it for other reasons?

Kristian-ZH commented 5 months ago

I think that this is achievable here as well. Just need to update the chart version here: https://github.com/suse-edge/charts/blob/main/packages/sriov-operator/generated-changes/patch/Chart.yaml.patch

For example, we need to bump it to 1.1.1

manuelbuil commented 5 months ago

I think that this is achievable here as well. Just need to update the chart version here: https://github.com/suse-edge/charts/blob/main/packages/sriov-operator/generated-changes/patch/Chart.yaml.patch

For example, we need to bump it to 1.1.1

I wanted to keep that semver in sync with the upstream version

manuelbuil commented 5 months ago

It's going to be tricky, I tried the packageVersion but for some reason it is ignored when building charts. I need to understand how we are doing it in rke2 (I know we use old scripts :P)

Kristian-ZH commented 5 months ago

Hmmm, in our charts, the version that is built is 1.1.0+up0.1.0 where the 1.1.0 is our chart version and the 0.1.0 is the upstream chart version. If we update our chart, it will become 1.1.1+up0.1.0 which will still keep the upstream version

Kristian-ZH commented 5 months ago

The version represents the version of the chart and the packageVersion represents the version of the application that is deployed in the chart (the image). These versions could be different and as we are referencing the upstream chart (not just the image) I think that it is better to use the version property of the Chart.yaml

manuelbuil commented 5 months ago

The problem is that upstream is never updating the chart version which stays always as 0.1.0. They are moving the appVersion which is what we are using as version in our Chart ==> https://github.com/suse-edge/charts/blob/main/packages/sriov-operator/generated-changes/patch/Chart.yaml.patch#L8. Therefore, if we use the version property of the Chart.yaml, we will lose track of the mapping between our version and upstream version, right?

manuelbuil commented 5 months ago

According to the docs, we could use packageVersion as a way to do it. We would have 1.1.000+up0.1.0 and then if we make a change without changing the upstream code: 1.1.001+up0.1.0. But I tried and it does not work like that, it is changing the patch version, e.g. with packageVersion: 56, I'm getting 1.1.56+up0.1.0. It feels like a bug!

manuelbuil commented 5 months ago

I have just checked the code and there is an exception when the patch is 0 (what we are seeing), it won't do 1.1.056 because having a 0 on the left side of the patch number is not a correct semver. In that case it does 1.1.56. Anyway, I just wanted to make you aware of these problems but I will follow along any rule you decide :)