Additional Deprecation Types #307
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #307 +/- ##
=======================================
Coverage 42.64% 42.64%
=======================================
Files 42 42
Lines 3583 3583
=======================================
Hits 1528 1528
Misses 1903 1903
Partials 152 152
☔ View full report in Codecov by Sentry. |
| @@ -118,8 +118,18 @@ const ( | |||
| // SubscriptionBundleUnpackFailed indicates that the unpack job failed | |||
| SubscriptionBundleUnpackFailed SubscriptionConditionType = "BundleUnpackFailed" | |||
|
|
|||
| // SubscriptionOperatorDeprecated indicates that the Operator currently installed with this Subscription has been deprecated. | |||
| // SubscriptionOperatorDeprecated is a roll-up condition which indicates that the Operator currently installed with this Subscription | |||
| //has been deprecated. It will be present when any of the three deprecation types (Package, Channel, Bundle) are present. | |||
| SubscriptionOperatorDeprecated SubscriptionConditionType = "OperatorDeprecated" | |||
There was a problem hiding this comment.
WDYT about renaming this one to just Deprecated?
I'm concerned that OperatorDeprecated sounds too much like BundleDeprecated and might confuse users.
There was a problem hiding this comment.
Agreed, I've updated to Deprecated
dtfranz
force-pushed
the
3098-deprecation-types
branch
from
78dc0a0 to
b60f97e
Compare
| // SubscriptionOperatorDeprecated indicates that the Package currently installed with this Subscription has been deprecated. | ||
| SubscriptionPackageDeprecated SubscriptionConditionType = "PackageDeprecated" | ||
|
|
||
| // SubscriptionOperatorDeprecated indicates that the Channel currently installed with this Subscription has been deprecated. |
There was a problem hiding this comment.
Nit: Channels aren't installable IIUC. Not serious enough to block this IMO.
| // SubscriptionOperatorDeprecated indicates that the Channel currently installed with this Subscription has been deprecated. | |
| // SubscriptionOperatorDeprecated indicates that the Channel used with this Subscription has been deprecated. |
There was a problem hiding this comment.
Woops, I'll correct this in a follow up. Thanks for catching.
EDIT: I went ahead and fixed it.
|
/lgtm |
…ndle). Signed-off-by: dtfranz <[email protected]>
dtfranz
force-pushed
the
3098-deprecation-types
branch
from
b60f97e to
6f8f2db
Compare
| SubscriptionOperatorDeprecated SubscriptionConditionType = "OperatorDeprecated" | ||
| // SubscriptionDeprecated is a roll-up condition which indicates that the Operator currently installed with this Subscription | ||
| //has been deprecated. It will be present when any of the three deprecation types (Package, Channel, Bundle) are present. | ||
| SubscriptionDeprecated SubscriptionConditionType = "Deprecated" |
There was a problem hiding this comment.
I’m not seeing the point of this condition. There are three distinct conditions that reflect which resource is deprecated. I don’t see the need of a blanket condition unless there is a usage for it that is not described in the comment or commit. This is why we should start to do proof PR to show how these fields are being used as a part of reviewing process.
There was a problem hiding this comment.
Or is this supposed to replace OperatorDeprecated?
There was a problem hiding this comment.
Yes, it's replacing OperatorDeprecated as the roll-up condition name. Having the roll-up allows us and any users to determine deprecated status without having to check for all three different sup-types.
| @@ -118,8 +118,18 @@ const ( | |||
| // SubscriptionBundleUnpackFailed indicates that the unpack job failed | |||
| SubscriptionBundleUnpackFailed SubscriptionConditionType = "BundleUnpackFailed" | |||
|
|
|||
| // SubscriptionOperatorDeprecated indicates that the Operator currently installed with this Subscription has been deprecated. | |||
There was a problem hiding this comment.
I suspect this condition is used on olm side. So this is kind of an api change. Is it necessary to remove this without deprecation warning?
There was a problem hiding this comment.
For context, we just added that field yesterday and it's not in any release, so it's not in use anywhere. We're making a correction to the name here and also adding sub-types so that users can see which levels their installed Operators are deprecated at.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtfranz, grokspawn 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-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 047dce1
into
operator-framework:master
Add additional deprecation conditions for each type (package, channel, bundle)