Handle CSV approvals with unconventional package names #385
Conversation
jan-law
force-pushed
the
csv-name
branch
4 times, most recently
from
f90cdb7 to
3d1cff2
Compare
| // Approve all CSVs that are dependencies of the subscription CSV. | ||
| for _, packageDependency := range packageDependencies.UnsortedList() { | ||
| for _, csvName := range installPlan.Spec.ClusterServiceVersionNames { | ||
| if strings.HasPrefix(csvName, packageDependency+".v") { |
There was a problem hiding this comment.
In another use case, if a dependent package also has another dependency with an unusual naming convention, I think the example redhat-another-operator package would be skipped during getBundleDependencies():
Contrived example Install plan bundles: [mtc-operator, oadp-operator, another-operator]
mtc-operator package (mtc-operator.v1.2.3) depends on
redhat-oadp-operator package (oadp-operator.v4.5.6), which depends on
redhat-another-operator package (another-operator.v7.8.9)
resulting value of packageDependencies: [redhat-oadp-operator]
Is this use case worth fixing? It's somewhat related to the original issue. The probability of such a case seems rare.
There was a problem hiding this comment.
Would fixing it be much more work? I agree it seems it's taken a bit to even find this scenario, but it'd also be nice to deliver something complete if it's not overly complex.
There was a problem hiding this comment.
Testing a fix for this now
There was a problem hiding this comment.
@dhaiducek Merged getBundleDependencies and removed all packageName+".v" assumptions. Ready for another round of reviews
| // Approve all CSVs that are dependencies of the subscription CSV. | ||
| for _, packageDependency := range packageDependencies.UnsortedList() { | ||
| for _, csvName := range installPlan.Spec.ClusterServiceVersionNames { | ||
| if strings.HasPrefix(csvName, packageDependency+".v") { |
There was a problem hiding this comment.
Would fixing it be much more work? I agree it seems it's taken a bit to even find this scenario, but it'd also be nice to deliver something complete if it's not overly complex.
| } | ||
|
|
||
| // getPackageToCSVMapping creates a map from olm package names to their CSV identifiers using bundle lookups | ||
| func getPackageToCSVMapping(installPlan *operatorv1alpha1.InstallPlan) map[string]string { |
There was a problem hiding this comment.
Should this function be merged into getBundleDependencies()? They seem to be doing very similar things.
There was a problem hiding this comment.
Good question. Would you know why getBundleDependencies() returns a list of package names instead of the CSVs?
Combining the two functions by returning a list of CSV names would simplify the lookup logic.
There was a problem hiding this comment.
I think it was an assumption that the package names would be the solution, but I don't see those results being used anywhere else so I suspect it ought to be fine to just update getBundleDependencies().
jan-law
force-pushed
the
csv-name
branch
3 times, most recently
from
df26903 to
c30e79d
Compare
ref: https://issues.redhat.com/browse/ACM-20500 Remove the assumption that dependency CSV names always match the format of <package name>.v<version>. When approving multiple CSVs in an install plan, inspect the bundles for required package names before recursively gathering dependency CSVs for approval. Signed-off-by: Janelle Law <[email protected]>
dhaiducek
left a comment
dhaiducek
left a comment
There was a problem hiding this comment.
Sorry, totally missed reviewing this yesterday! LGTM!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, jan-law The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit b76922e
into
open-cluster-management-io:main
2 participants
ref: https://issues.redhat.com/browse/ACM-20500
When an operator policy enforces automatic upgrades, dependency CSVs should be automatically approved. In some cases, CSVs do not match the name format of
<package name>.v<version>and could not be approved.The PR looks up the CSV name for each dependency package by reading the install plan.