Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 don't consider bundle deprecated if the package is deprecated #577

Merged
merged 2 commits into from
Jan 17, 2024
Merged

🌱 don't consider bundle deprecated if the package is deprecated #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ab95bb
(fix): don't consider bundle deprecated if the package is deprecated
everettraven Jan 17, 2024
17799af
update IsDeprecated() comment
everettraven Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions internal/catalogmetadata/sort/sort_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,32 @@ func TestByDeprecated(t *testing.T) {
require.Len(t, toSort, 2)
assert.Equal(t, b2, toSort[0])
assert.Equal(t, b1, toSort[1])

b1.Deprecations = []declcfg.DeprecationEntry{
{
Reference: declcfg.PackageScopedReference{
Schema: "olm.package",
},
},
}
b2.Deprecations = append(b2.Deprecations, declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.package",
},
}, declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.bundle",
Name: "baz",
},
})

toSort = []*catalogmetadata.Bundle{b2, b1}
sort.SliceStable(toSort, func(i, j int) bool {
return catalogsort.ByDeprecated(toSort[i], toSort[j])
})
// Both are deprecated at package level, b2 is deprecated
// explicitly, b2 should be preferred less
require.Len(t, toSort, 2)
assert.Equal(t, b1, toSort[0])
assert.Equal(t, b2, toSort[1])
}
16 changes: 6 additions & 10 deletions internal/catalogmetadata/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,19 +157,15 @@ func (b *Bundle) HasDeprecation() bool {

// IsDeprecated returns true if the bundle
// has been explicitly deprecated. This can occur
// in one of two ways:
// - the olm.package the bundle belongs to has been deprecated
// - the bundle itself has been deprecated
// if the bundle itself has been deprecated.
// this function does not take into consideration
// olm.channel deprecations associated with the bundle
// as a bundle can be present in multiple channels with
// some channels being deprecated and some not.
// olm.channel or olm.package deprecations associated
// with the bundle as a bundle can be present in multiple
// channels with some channels being deprecated and some not
// Package deprecation does not carry the same meaning as an individual
// bundle deprecation, so package deprecation is not considered.
func (b *Bundle) IsDeprecated() bool {
for _, dep := range b.Deprecations {
if dep.Reference.Schema == declcfg.SchemaPackage && dep.Reference.Name == b.Package {
return true
}

Comment on lines -169 to -172
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructuring resolution to return variables for Package, Channel, and Bundle types is out of scope for this work IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 foo is deprecated
  • Bundle foo.v1.0.0 is not deprecated
  • Bundle foo.v1.0.1 is 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay. I was under the impression that in that scenario foo.v1.0.1 would still be preferred over foo.v1.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6fdc060 - reverts to where the check is removed and should satisfy the expectation you have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joelanford Could you take another look when you have some time?

if dep.Reference.Schema == declcfg.SchemaBundle && dep.Reference.Name == b.Name {
return true
}
Expand Down
52 changes: 52 additions & 0 deletions internal/catalogmetadata/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,3 +229,55 @@ func TestBundleHasDeprecation(t *testing.T) {
})
}
}

func TestBundleIsDeprecated(t *testing.T) {
for _, tt := range []struct {
name string
bundle *catalogmetadata.Bundle
deprecated bool
}{
{
name: "has package and channel deprecations, not deprecated",
bundle: &catalogmetadata.Bundle{
Deprecations: []declcfg.DeprecationEntry{
{
Reference: declcfg.PackageScopedReference{
Schema: "olm.package",
},
},
{
Reference: declcfg.PackageScopedReference{
Schema: "olm.channel",
Name: "foo",
},
},
},
},
},
{
name: "has no deprecation entries, not deprecated",
bundle: &catalogmetadata.Bundle{},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this question is related to #577 (comment)

},
{
name: "has bundle deprecation entry, deprecated",
bundle: &catalogmetadata.Bundle{
Bundle: declcfg.Bundle{
Name: "foo",
},
Deprecations: []declcfg.DeprecationEntry{
{
Reference: declcfg.PackageScopedReference{
Schema: "olm.bundle",
Name: "foo",
},
},
},
},
deprecated: true,
},
} {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.deprecated, tt.bundle.IsDeprecated())
})
}
}
4 changes: 2 additions & 2 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,10 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad
}

// There are two early return scenarios here:
// 1) The bundle is not deprecated (i.e no package or bundle deprecations)
// 1) The bundle is not deprecated (i.e bundle deprecations)
// AND there are no other deprecations associated with the bundle
// 2) The bundle is not deprecated, there are deprecations associated
// with the bundle (i.e at least one channel the bundle is present in is deprecated),
// with the bundle (i.e at least one channel the bundle is present in is deprecated OR whole package is deprecated),
// and the ClusterExtension does not specify a channel. This is because the channel deprecations
// are a loose deprecation coupling on the bundle. A ClusterExtension installation is only
// considered deprecated by a channel deprecation when a deprecated channel is specified via
Expand Down