🌱 don't consider bundle deprecated if the package is deprecated #577
Conversation
| if dep.Reference.Schema == declcfg.SchemaPackage && dep.Reference.Name == b.Package { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
Removing this will actually mean that explicitly deprecated bundles will be preferred less than ones that aren't even if the entire package is deprecated - fixing
There was a problem hiding this comment.
Looking at this a little closer (sorry I didn't get the chance with the original PR), I'm a little concerned that we're putting package and channel level deprecations in the Bundle type.
I think I would have expected that we would put the package deprecation in the Package type, channel deprecation in the Channel type and bundle deprecation in the Bundle type.
And if we had that, we could use composition by embedding the deprecation type in each of the package/channel/bundle types and then use a shared IsDeprecated function that comes with that deprecation type.
Is it possible to do something like this?
There was a problem hiding this comment.
Probably, but my concern with doing that is that we would likely need to restructure resolution to provide all those types as results from the resolution. IIUC the resolution process currently will only return the Bundle type that contains all the necessary information for continuing with the install/upgrade process.
There was a problem hiding this comment.
Restructuring resolution to return variables for Package, Channel, and Bundle types is out of scope for this work IMO
There was a problem hiding this comment.
Removing this will actually mean that explicitly deprecated bundles will be preferred less than ones that aren't even if the entire package is deprecated - fixing
One other question about this point. Think about this scenario:
- Package
foois deprecated - Bundle
foo.v1.0.0is not deprecated - Bundle
foo.v1.0.1is deprecated
I would expect foo.v1.0.0 to be preferred over foo.v1.0.1. The package deprecation should not cascade to the bundles one way or the other.
There was a problem hiding this comment.
Oh okay. I was under the impression that in that scenario foo.v1.0.1 would still be preferred over foo.v1.0.0
There was a problem hiding this comment.
6fdc060 - reverts to where the check is removed and should satisfy the expectation you have.
There was a problem hiding this comment.
@joelanford Could you take another look when you have some time?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
+ Coverage 84.35% 84.83% +0.48%
==========================================
Files 20 20
Lines 933 930 -3
==========================================
+ Hits 787 789 +2
+ Misses 102 98 -4
+ Partials 44 43 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
everettraven
force-pushed
the
fix/remove-package-deprecation-influence
branch
from
ccb4a31
to
6f651c7
Compare
Signed-off-by: everettraven <everettraven@gmail.com>
everettraven
force-pushed
the
fix/remove-package-deprecation-influence
branch
from
7ee8dc9
to
1ab95bb
Compare
Signed-off-by: everettraven <everettraven@gmail.com>
| }, | ||
| { | ||
| name: "has no deprecation entries, not deprecated", | ||
| bundle: &catalogmetadata.Bundle{}, |
There was a problem hiding this comment.
Possibly a silly question: Does this catalogmetadata.Bundle{} need to have the package and channel deprecations listed for them to show up in the ClusterExtension deprecation status conditions, or is that handled in a separate data structure?
If it is handled in a separate data strucutre, could we change catalogmetadata.Bundle to contain something like:
package catalogmetadata
type Bundle struct {
...
deprecated bool
}
It seems unnecessary to store a list of Deprecations where there will always be 0 or 1 and the schema/name will always be olm.bundle and bundle.Name.
There was a problem hiding this comment.
Does this catalogmetadata.Bundle{} need to have the package and channel deprecations listed for them to show up in the ClusterExtension deprecation status conditions
Correct.
or is that handled in a separate data structure?
Not handled in a separate data structure at the moment as the catalogmetadata.Bundle type is what is used to determine everything after resolution.
There was a problem hiding this comment.
I think this question is related to #577 (comment)
Description
catalogmetadata.Bundle.IsDeprecated()method to no longer return true if there is a package deprecation associated with the bundlecatalogmetadata.Bundle.IsDeprecated()methodReviewer Checklist