Add deprecation status in operatorpolicy #327
Conversation
yiraeChristineKim
force-pushed
the
ACM-16120
branch
7 times, most recently
from
2feb6c6 to
284d018
Compare
yiraeChristineKim
force-pushed
the
ACM-16120
branch
10 times, most recently
from
be32a57 to
560eb06
Compare
yiraeChristineKim
force-pushed
the
ACM-16120
branch
from
560eb06 to
4afd739
Compare
There was a problem hiding this comment.
This is looking great overall! Awesome to see the fake operator for this.
In addition to the "nothing deprecated" condition I mentioned, we should conditionally report the deprecation in the overall status:
If you'd prefer, I'd be ok with that addition in a separate PR.
so my understanding what you suggest, should deprecation status affect compliance(NoCompliant or Compliant)? |
yiraeChristineKim
force-pushed
the
ACM-16120
branch
from
4afd739 to
14bafa5
Compare
| // deprecationCond returns a Compliant condition (without affecting the condition itself), | ||
| // and a message in the format: 'The ____ (Package, Channel, Bundle) was deprecated.' | ||
| // + additional details from packageManifest. | ||
| func deprecationCond(depSchemaName string, depSchema Schema, message string) metav1.Condition { |
There was a problem hiding this comment.
If we don't want this to affect the final compliance, we should ensure that this returns a Status that matches whatever is Compliant. Right now it doesn't.
To allow users to determine if they want the deprecation status to affect the overall compliance, we'll need to add something to the ComplianceConfig, and pass that into this function. For example, buildDeploymentCond uses that pattern.
There was a problem hiding this comment.
I see what you mean
There was a problem hiding this comment.
Could you take a look my change
OperatorPolicy is defined for an operator which is either running a deprecated version, or a channel for updates which is deprecated, the policy should report this in its status ref: https://issues.redhat.com/browse/ACM-16120 Signed-off-by: yiraeChristineKim <[email protected]>
1. Fix host test flaky issue. case38:3702 Signed-off-by: yiraeChristineKim <[email protected]>
yiraeChristineKim
force-pushed
the
ACM-16120
branch
2 times, most recently
from
6533e49 to
f548dae
Compare
api/v1beta1/operatorpolicy_types.go
Outdated
| // DeprecationAvailable specifies how the NoDeprecations condition impacts overall | ||
| // policy compliance. The default value is `Compliant`. If any deprecations are detected | ||
| // in the package, channel, or bundle with DeprecationAvailable = NonCompliant, | ||
| // then the policy compliance will be set to `Compliant`. | ||
| // | ||
| // +kubebuilder:default=Compliant | ||
| DeprecationAvailable ComplianceConfigAction `json:"deprecationAvailable,omitempty"` |
There was a problem hiding this comment.
Maybe DeprecationsPresent would make this more clear?
| // DeprecationAvailable specifies how the NoDeprecations condition impacts overall | |
| // policy compliance. The default value is `Compliant`. If any deprecations are detected | |
| // in the package, channel, or bundle with DeprecationAvailable = NonCompliant, | |
| // then the policy compliance will be set to `Compliant`. | |
| // | |
| // +kubebuilder:default=Compliant | |
| DeprecationAvailable ComplianceConfigAction `json:"deprecationAvailable,omitempty"` | |
| // DeprecationsPresent specifies how the overall policy compliance is affected by deprecations. | |
| // The default value is `Compliant`. If any deprecations are detected | |
| // while DeprecationsPresent = NonCompliant, | |
| // then the policy compliance will be set to `NonCompliant`. | |
| // | |
| // +kubebuilder:default=Compliant | |
| DeprecationsPresent ComplianceConfigAction `json:"deprecationsPresent,omitempty"` |
controllers/operatorpolicy_status.go
Outdated
|
|
||
| idx, cond = policy.Status.GetCondition(deprecationType) | ||
| if !policy.Spec.ComplianceType.IsMustNotHave() && | ||
| policy.Spec.ComplianceConfig.DeprecationAvailable == "Compliant" { |
There was a problem hiding this comment.
Other conditions change the Status of the condition based on the complianceConfig, but I feel like your approach here is better.
If you follow my other suggestion, I think this part would need to be updated like this:
| policy.Spec.ComplianceConfig.DeprecationAvailable == "Compliant" { | |
| policy.Spec.ComplianceConfig.DeprecationsPresent == "NonCompliant" { |
yiraeChristineKim
force-pushed
the
ACM-16120
branch
from
f548dae to
cdc6c1d
Compare
Add DeprecationsPresent to ComplianceConfigAction Signed-off-by: yiraeChristineKim <[email protected]>
yiraeChristineKim
force-pushed
the
ACM-16120
branch
from
cdc6c1d to
7f237ff
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, yiraeChristineKim 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-merge-bot
bot
merged commit c3c1716
into
open-cluster-management-io:main
OperatorPolicy is defined for an operator which is either running a deprecated version, or a channel for updates which is deprecated, the policy should report this in its status
Additionally, resolve the flaky OperatorPolicy issue by switching from the Quay operator to the GRC fake operator.
ref: https://issues.redhat.com/browse/ACM-16120
Signed-off-by: yiraeChristineKim [email protected]