Fix status reporting inconsistency in mustnothave mode #240
Conversation
|
@JustinKuli This is pretty much what I had in mind for fixing consistency issues around the relatedObjects for deployments and catalogsources. I realized now that these changes might hurt the readability of the code in exchange for reporting finer details in the relatedObjects field. A simpler alternative is to just create a more "barebones" relatedObject that tells users the controller doesn't handle these objects in mustnothavemode, without caring too much about the granular details (like the name/namespace where the object can be found in). Let me know what you think! |
controllers/operatorpolicy_status.go
Outdated
| // reason = "Resource not found but should exist"/"Resource not found as expected" | ||
| // based on the complianceType specified by the policy | ||
| // Note: These relatedObjects are not applicable *only* in mustnothave mode | ||
| func missingNotApplicableObj(name string, namespace string, complianceType policyv1.ComplianceType, gvk schema.GroupVersionKind) policyv1.RelatedObject { |
There was a problem hiding this comment.
Can this be renamed to just missingObj? Since it doesn't use the NotApplicable reason, it seems odd.
| // Mustnothave | ||
| var relObj policyv1.RelatedObject | ||
| cond := notApplicableCond("CatalogSource") | ||
| cond.Status = metav1.ConditionFalse // CatalogSource condition has the opposite polarity | ||
|
|
||
| if isMissing { | ||
| relObj = missingNotApplicableObj(catalogName, catalogNS, policyv1.MustNotHave, catalogSrcGVK) | ||
| } else { | ||
| relObj = foundNotApplicableObj(catalogSrc) | ||
| } | ||
|
|
||
| changed := updateStatus(policy, catalogSourceFindCond(isUnhealthy, isMissing, catalogName), | ||
| catalogSourceObj(catalogName, catalogNS, isUnhealthy, isMissing)) | ||
| changed := updateStatus(policy, cond, relObj) |
There was a problem hiding this comment.
Since mustnothave mode does not need to check if the catalogsource is healthy, can it be handled earlier? I have a feeling it would clean up some of this. Right now, in particular, I needed to double-check that the catalogSrc here wasn't a nil pointer - but I think it could've just used foundCatalogSrc?
|
|
zyjjay
force-pushed
the
ACM-11410
branch
4 times, most recently
from
4bf768b
to
817e570
Compare
Changes: - Added new conditions and related objects to account for scenarios where resources are not handled in mustnothave mode - Modified and refactored code for handling CatalogSource and Deployment - Modified e2e test to account for status reporting changes ref: https://issues.redhat.com/browse/ACM-11410 Signed-off-by: Jason Zhang <jaszhang@redhat.com>
zyjjay
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, zyjjay 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 e60ff79
into
open-cluster-management-io:main
Mostly for handling of deployments and catalogsources