✨ (feat): add graph deprecation logic #574

everettraven · 2024-01-08T20:26:42Z

Description

Add new condition types and reasons to signal when an installed bundle for a ClusterExtension has deprecations associated with it
Update the catalogmetadata.Bundle type to have fields to hold the deprecation information for a bundle
Update the resolution logic to prefer bundles with deprecations less than bundles without
Update the ClusterExtension reconciler logic to add the deprecation statuses if there are any associated with the installed bundle

Motivation

Implements RFC: OLMv1 Graph Deprecation
fixes (Placeholder): Add deprecation information to bundles/bundle variables #525
fixes (Placeholder): Add new status condition to convey deprecation status of Operator #526
fixes (Placeholder): When ForceSemverUpgradeConstraints=false prefer deprecated bundles less during install/upgrade #527

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

internal/catalogmetadata/client/client.go

internal/controllers/clusterextension_controller.go

internal/resolution/variablesources/required_package_test.go

codecov · 2024-01-08T21:21:26Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (457010e) 83.60% compared to head (678c58e) 84.35%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
internal/catalogmetadata/types.go	28.57%	4 Missing and 1 partial ⚠️
...nternal/controllers/clusterextension_controller.go	94.66%	4 Missing ⚠️
internal/catalogmetadata/client/client.go	91.66%	1 Missing and 1 partial ⚠️
internal/catalogmetadata/sort/sort.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
+ Coverage   83.60%   84.35%   +0.75%     
==========================================
  Files          20       20              
  Lines         811      933     +122     
==========================================
+ Hits          678      787     +109     
- Misses         92      102      +10     
- Partials       41       44       +3

Flag	Coverage Δ
e2e	`60.23% <39.02%> (-3.27%)`	⬇️
unit	`80.09% <88.61%> (+1.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dtfranz · 2024-01-09T18:58:35Z

internal/catalogmetadata/types.go

@@ -140,6 +145,10 @@ func (b *Bundle) propertiesByType(propType string) []*property.Property {
 	return b.propertiesMap[propType]
 }

+func (b *Bundle) HasDeprecation() bool {


Nit-picky but maybe:

Suggested change

func (b *Bundle) HasDeprecation() bool {

func (b *Bundle) IsDeprecated() bool {

reads a little better to me.

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.

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().

Sounds like a good place to add GoDoc :)

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.

cc @joelanford @grokspawn ^

I've updated the implementation in 9186d16 to avoid the scenario I mentioned above

dtfranz

You've taken care of my concerns here and also handled the channel deprecation case well in my opinion, thanks!
I'll hold off on explicit approval for now just to make sure that some of the other reviewers get a chance to look at things :)

to signal when an installed bundle has deprecations associated with it and prefer bundles with deprecations less than non-deprecated bundles Signed-off-by: everettraven <everettraven@gmail.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2024

everettraven changed the title ~~wip: (feat): add graph deprecation logic~~ ✨ wip: (feat): add graph deprecation logic Jan 8, 2024

everettraven commented Jan 8, 2024

View reviewed changes

internal/catalogmetadata/client/client.go Outdated Show resolved Hide resolved

everettraven commented Jan 8, 2024

View reviewed changes

internal/catalogmetadata/client/client.go Outdated Show resolved Hide resolved

everettraven commented Jan 8, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

everettraven commented Jan 8, 2024

View reviewed changes

internal/resolution/variablesources/required_package_test.go Show resolved Hide resolved

everettraven changed the title ~~✨ wip: (feat): add graph deprecation logic~~ ✨ (feat): add graph deprecation logic Jan 8, 2024

everettraven marked this pull request as ready for review January 8, 2024 21:36

everettraven requested a review from a team as a code owner January 8, 2024 21:36

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2024

dtfranz reviewed Jan 9, 2024

View reviewed changes

everettraven force-pushed the feature/graph-deprecation branch from f91c589 to 33de0a6 Compare January 11, 2024 14:44

everettraven requested review from stevekuznetsov and dtfranz January 11, 2024 19:55

dtfranz reviewed Jan 11, 2024

View reviewed changes

(feat): add graph deprecation logic

678c58e

to signal when an installed bundle has deprecations associated with it and prefer bundles with deprecations less than non-deprecated bundles Signed-off-by: everettraven <everettraven@gmail.com>

everettraven force-pushed the feature/graph-deprecation branch from 201244b to 678c58e Compare January 12, 2024 14:51

everettraven enabled auto-merge January 12, 2024 15:12

stevekuznetsov approved these changes Jan 12, 2024

View reviewed changes

everettraven added this pull request to the merge queue Jan 12, 2024

Merged via the queue into operator-framework:main with commit 1e6ad96 Jan 12, 2024
15 checks passed

ncdc mentioned this pull request Jan 24, 2024

Support deprecating channels #389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ (feat): add graph deprecation logic #574

✨ (feat): add graph deprecation logic #574

everettraven commented Jan 8, 2024

codecov bot commented Jan 8, 2024 •

edited

Loading

dtfranz Jan 9, 2024

everettraven Jan 9, 2024

dtfranz Jan 9, 2024

stevekuznetsov Jan 11, 2024

everettraven Jan 11, 2024

everettraven Jan 11, 2024 •

edited

Loading

everettraven Jan 11, 2024

everettraven Jan 11, 2024

dtfranz left a comment

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

✨ (feat): add graph deprecation logic #574

✨ (feat): add graph deprecation logic #574

Conversation

everettraven commented Jan 8, 2024

Description

Motivation

Reviewer Checklist

codecov bot commented Jan 8, 2024 • edited Loading

Codecov Report

dtfranz Jan 9, 2024

Choose a reason for hiding this comment

everettraven Jan 9, 2024

Choose a reason for hiding this comment

dtfranz Jan 9, 2024

Choose a reason for hiding this comment

stevekuznetsov Jan 11, 2024

Choose a reason for hiding this comment

everettraven Jan 11, 2024

Choose a reason for hiding this comment

everettraven Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

everettraven Jan 11, 2024

Choose a reason for hiding this comment

everettraven Jan 11, 2024

Choose a reason for hiding this comment

dtfranz left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2024 •

edited

Loading

everettraven Jan 11, 2024 •

edited

Loading