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

✨ (feat): add graph deprecation logic #574

Merged
merged 1 commit into from
Jan 12, 2024
Merged

✨ (feat): add graph deprecation logic #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ const (
// TODO(user): add more Types, here and into init()
TypeInstalled = "Installed"
TypeResolved = "Resolved"
// TypeDeprecated is a rollup condition that is present when
// any of the deprecated conditions are present.
TypeDeprecated = "Deprecated"
TypePackageDeprecated = "PackageDeprecated"
TypeChannelDeprecated = "ChannelDeprecated"
TypeBundleDeprecated = "BundleDeprecated"

ReasonBundleLookupFailed = "BundleLookupFailed"
ReasonInstallationFailed = "InstallationFailed"
Expand All @@ -80,13 +86,18 @@ const (
ReasonResolutionFailed = "ResolutionFailed"
ReasonResolutionUnknown = "ResolutionUnknown"
ReasonSuccess = "Success"
ReasonDeprecated = "Deprecated"
)

func init() {
// TODO(user): add Types from above
conditionsets.ConditionTypes = append(conditionsets.ConditionTypes,
TypeInstalled,
TypeResolved,
TypeDeprecated,
TypePackageDeprecated,
TypeChannelDeprecated,
TypeBundleDeprecated,
)
// TODO(user): add Reasons from above
conditionsets.ConditionReasons = append(conditionsets.ConditionReasons,
Expand All @@ -98,6 +109,7 @@ func init() {
ReasonInstallationStatusUnknown,
ReasonInvalidSpec,
ReasonSuccess,
ReasonDeprecated,
)
}

Expand Down
13 changes: 10 additions & 3 deletions cmd/resolutioncli/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func (c *indexRefClient) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle
}

var (
channels []*catalogmetadata.Channel
bundles []*catalogmetadata.Bundle
channels []*catalogmetadata.Channel
bundles []*catalogmetadata.Bundle
deprecations []*catalogmetadata.Deprecation
)

for i := range cfg.Channels {
Expand All @@ -63,10 +64,16 @@ func (c *indexRefClient) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle
})
}

for i := range cfg.Deprecations {
deprecations = append(deprecations, &catalogmetadata.Deprecation{
Deprecation: cfg.Deprecations[i],
})
}

// TODO: update fake catalog name string to be catalog name once we support multiple catalogs in CLI
catalogName := "offline-catalog"

bundles, err = client.PopulateExtraFields(catalogName, channels, bundles)
bundles, err = client.PopulateExtraFields(catalogName, channels, bundles, deprecations)
if err != nil {
return nil, err
}
Expand Down
41 changes: 39 additions & 2 deletions internal/catalogmetadata/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
}
channels := []*catalogmetadata.Channel{}
bundles := []*catalogmetadata.Bundle{}
deprecations := []*catalogmetadata.Deprecation{}

rc, err := c.fetcher.FetchCatalogContents(ctx, catalog.DeepCopy())
if err != nil {
Expand All @@ -81,6 +82,12 @@ func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
return fmt.Errorf("error unmarshalling bundle from catalog metadata: %s", err)
}
bundles = append(bundles, &content)
case declcfg.SchemaDeprecation:
var content catalogmetadata.Deprecation
if err := json.Unmarshal(meta.Blob, &content); err != nil {
return fmt.Errorf("error unmarshalling deprecation from catalog metadata: %s", err)
}
deprecations = append(deprecations, &content)
}

return nil
Expand All @@ -89,7 +96,7 @@ func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
return nil, fmt.Errorf("error processing response: %s", err)
}

bundles, err = PopulateExtraFields(catalog.Name, channels, bundles)
bundles, err = PopulateExtraFields(catalog.Name, channels, bundles, deprecations)
if err != nil {
return nil, err
}
Expand All @@ -100,7 +107,7 @@ func (c *Client) Bundles(ctx context.Context) ([]*catalogmetadata.Bundle, error)
return allBundles, nil
}

func PopulateExtraFields(catalogName string, channels []*catalogmetadata.Channel, bundles []*catalogmetadata.Bundle) ([]*catalogmetadata.Bundle, error) {
func PopulateExtraFields(catalogName string, channels []*catalogmetadata.Channel, bundles []*catalogmetadata.Bundle, deprecations []*catalogmetadata.Deprecation) ([]*catalogmetadata.Bundle, error) {
bundlesMap := map[string]*catalogmetadata.Bundle{}
for i := range bundles {
bundleKey := fmt.Sprintf("%s-%s", bundles[i].Package, bundles[i].Name)
Expand All @@ -121,5 +128,35 @@ func PopulateExtraFields(catalogName string, channels []*catalogmetadata.Channel
}
}

// According to https://docs.google.com/document/d/1EzefSzoGZL2ipBt-eCQwqqNwlpOIt7wuwjG6_8ZCi5s/edit?usp=sharing
// the olm.deprecations FBC object is only valid when either 0 or 1 instances exist
// for any given package
deprecationMap := make(map[string]*catalogmetadata.Deprecation, len(deprecations))
for _, deprecation := range deprecations {
deprecationMap[deprecation.Package] = deprecation
}

for i := range bundles {
if dep, ok := deprecationMap[bundles[i].Package]; ok {
for _, entry := range dep.Entries {
switch entry.Reference.Schema {
case declcfg.SchemaPackage:
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
case declcfg.SchemaChannel:
for _, ch := range bundles[i].InChannels {
if ch.Name == entry.Reference.Name {
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
break
}
}
case declcfg.SchemaBundle:
if bundles[i].Name == entry.Reference.Name {
bundles[i].Deprecations = append(bundles[i].Deprecations, entry)
}
}
}
}
}

return bundles, nil
}
39 changes: 39 additions & 0 deletions internal/catalogmetadata/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,45 @@ func TestClient(t *testing.T) {
},
fetcher: &MockFetcher{},
},
{
name: "deprecated at the package, channel, and bundle level",
fakeCatalog: func() ([]client.Object, []*catalogmetadata.Bundle, map[string][]byte) {
objs, bundles, catalogContentMap := defaultFakeCatalog()

catalogContentMap["catalog-1"] = append(catalogContentMap["catalog-1"],
[]byte(`{"schema": "olm.deprecations", "package":"fake1", "entries":[{"message": "fake1 is deprecated", "reference": {"schema": "olm.package"}}, {"message":"channel stable is deprecated", "reference": {"schema": "olm.channel", "name": "stable"}}, {"message": "bundle fake1.v1.0.0 is deprecated", "reference":{"schema":"olm.bundle", "name":"fake1.v1.0.0"}}]}`)...)

for i := range bundles {
if bundles[i].Package == "fake1" && bundles[i].CatalogName == "catalog-1" && bundles[i].Name == "fake1.v1.0.0" {
bundles[i].Deprecations = append(bundles[i].Deprecations, declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.package",
},
Message: "fake1 is deprecated",
})

bundles[i].Deprecations = append(bundles[i].Deprecations, declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.channel",
Name: "stable",
},
Message: "channel stable is deprecated",
})

bundles[i].Deprecations = append(bundles[i].Deprecations, declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.bundle",
Name: "fake1.v1.0.0",
},
Message: "bundle fake1.v1.0.0 is deprecated",
})
}
}

return objs, bundles, catalogContentMap
},
fetcher: &MockFetcher{},
},
} {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()
Expand Down
6 changes: 6 additions & 0 deletions internal/catalogmetadata/filter/bundle_predicates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,9 @@ func Replaces(bundleName string) Predicate[catalogmetadata.Bundle] {
return false
}
}

func WithDeprecation(deprecated bool) Predicate[catalogmetadata.Bundle] {
return func(bundle *catalogmetadata.Bundle) bool {
return bundle.HasDeprecation() == deprecated
}
}
16 changes: 16 additions & 0 deletions internal/catalogmetadata/filter/bundle_predicates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,19 @@ func TestReplaces(t *testing.T) {
assert.False(t, f(b2))
assert.False(t, f(b3))
}

func TestWithDeprecation(t *testing.T) {
b1 := &catalogmetadata.Bundle{
Deprecations: []declcfg.DeprecationEntry{
{
Reference: declcfg.PackageScopedReference{},
},
},
}

b2 := &catalogmetadata.Bundle{}

f := filter.WithDeprecation(true)
assert.True(t, f(b1))
assert.False(t, f(b2))
}
19 changes: 19 additions & 0 deletions internal/catalogmetadata/sort/sort.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@ func ByVersion(b1, b2 *catalogmetadata.Bundle) bool {
return ver1.GT(*ver2)
}

// ByDeprecation is a sort "less" function that orders bundles
// that are deprecated lower than ones without deprecations
func ByDeprecated(b1, b2 *catalogmetadata.Bundle) bool {
b1Val := 1
b2Val := 1

if b1.IsDeprecated() {
b1Val = b1Val - 1
}

if b2.IsDeprecated() {
b2Val = b2Val - 1
}

// Check for "greater than" because we
// non deprecated on top
return b1Val > b2Val
}

// compareErrors returns 0 if both errors are either nil or not nil
// -1 if err1 is nil and err2 is not nil
// +1 if err1 is not nil and err2 is nil
Expand Down
51 changes: 51 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 @@ -6,6 +6,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/operator-framework/operator-registry/alpha/declcfg"
"github.com/operator-framework/operator-registry/alpha/property"
Expand Down Expand Up @@ -81,3 +82,53 @@ func TestByVersion(t *testing.T) {
assert.Equal(t, b5empty, toSort[4])
})
}

func TestByDeprecated(t *testing.T) {
b1 := &catalogmetadata.Bundle{
CatalogName: "foo",
Bundle: declcfg.Bundle{
Name: "bar",
},
}

b2 := &catalogmetadata.Bundle{
CatalogName: "foo",
Bundle: declcfg.Bundle{
Name: "baz",
},
Deprecations: []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])
})

require.Len(t, toSort, 2)
assert.Equal(t, b1, toSort[0])
assert.Equal(t, b2, toSort[1])

// Channel deprecation association != bundle deprecated
b2.Deprecations[0] = declcfg.DeprecationEntry{
Reference: declcfg.PackageScopedReference{
Schema: "olm.channel",
Name: "badchannel",
},
}

toSort = []*catalogmetadata.Bundle{b2, b1}
sort.SliceStable(toSort, func(i, j int) bool {
return catalogsort.ByDeprecated(toSort[i], toSort[j])
})
// No bundles are deprecated so ordering should remain the same
require.Len(t, toSort, 2)
assert.Equal(t, b2, toSort[0])
assert.Equal(t, b1, toSort[1])
}
44 changes: 41 additions & 3 deletions internal/catalogmetadata/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const (
)

type Schemas interface {
Package | Bundle | Channel
Package | Bundle | Channel | Deprecation
}

type Package struct {
Expand All @@ -29,15 +29,20 @@ type Channel struct {
declcfg.Channel
}

type Deprecation struct {
declcfg.Deprecation
}

type PackageRequired struct {
property.PackageRequired
SemverRange bsemver.Range `json:"-"`
}

type Bundle struct {
declcfg.Bundle
CatalogName string
InChannels []*Channel
CatalogName string
InChannels []*Channel
Deprecations []declcfg.DeprecationEntry

mu sync.RWMutex
// these properties are lazy loaded as they are requested
Expand Down Expand Up @@ -140,6 +145,39 @@ func (b *Bundle) propertiesByType(propType string) []*property.Property {
return b.propertiesMap[propType]
}

// HasDeprecation returns true if the bundle
// has any deprecations associated with it.
// This may return true even in cases where the bundle
// may be associated with an olm.channel deprecation
// but the bundle is not considered "deprecated" because
// the bundle is selected via a non-deprecated channel.
func (b *Bundle) HasDeprecation() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-picky but maybe:

Suggested change
func (b *Bundle) HasDeprecation() bool {
func (b *Bundle) IsDeprecated() bool {

reads a little better to me.

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 can see that. Reason I went with HasDeprecation is more so because of the potentially confusing classification of "Deprecated" in this case. If a channel is deprecated, that doesn't necessarily mean every bundle in that channel is deprecated. The ClusterExtension would be marked deprecated if it specifically asks for that channel but otherwise that deprecation notice wouldn't show up. Due to this I felt HasDeprecation was more like asking "Are there any deprecations that are associated with this bundle?" rather than "Is this bundle deprecated?"

That being said, I'm not opposed to changing this. I do feel that IsDeprecated() could be a bit misleading without having a deeper understanding of how the deprecation concept works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good point, and as you noted I myself also misunderstood the concept here by thinking that all bundles under a deprecated channel were also considered deprecated. If that's not the case then I agree with HasDeprecation().

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good place to add GoDoc :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a2ff3ff

Copy link
Contributor Author

@everettraven everettraven Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

One thing to point out with this that I thought about while writing the GoDoc is that during resolution if the bundle is associated with any deprecations it will be less preferred over other bundles even if it is an olm.channel deprecation that isn't being considered due to the specification of using a different channel.

For example, given the following set of bundles in a desired channel stable:

foo.v1.0.0
foo.v1.1.0
foo.v2.0.0
foo.v2.1.0
foo.v3.0.0 <-- associated with a deprecated channel named stable-v3 due to $reasons

The resulting resolution list in order of most preferred -> least preferred would be:

foo.v2.1.0
foo.v2.0.0
foo.v1.1.0
foo.v1.0.0
foo.v3.0.0

This feels like a scenario we may want to avoid by having a distinction between a bundle having explicit deprecation due to the package or itself being deprecated and the looser deprecation associated with the bundle being an entry within a deprecated channel

Do other folks feel this scenario should be avoided as well? I could also see an argument for preferring any bundle with associated deprecations than ones with none, and if that is the functionality we want then no changes would be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @joelanford @grokspawn ^

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've updated the implementation in 9186d16 to avoid the scenario I mentioned above

return len(b.Deprecations) > 0
}

// 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
// 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.
func (b *Bundle) IsDeprecated() bool {
for _, dep := range b.Deprecations {
if dep.Reference.Schema == declcfg.SchemaPackage && dep.Reference.Name == b.Package {
return true
}

if dep.Reference.Schema == declcfg.SchemaBundle && dep.Reference.Name == b.Name {
return true
}
}

return false
}

func loadOneFromProps[T any](bundle *Bundle, propType string, required bool) (T, error) {
r, err := loadFromProps[T](bundle, propType, required)
if err != nil {
Expand Down
Loading