-
Notifications
You must be signed in to change notification settings - Fork 18
Fix operatorpolicy confusing status when invalid #413
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.
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
Fix operatorpolicy confusing status when invalid #413
Conversation
|
/ok-to-test |
e9a6b18 to
fb4b3cd
Compare
|
/cc @dhaiducek |
JustinKuli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting to see some change around where buildResources is called, or where the full compliance message is created - how does this PR shorten the full message to only the relevant part? It seems like when the subscription or operator group are invalid (for example, specifying installPlanApproval), the compliance message will still include the repetitive/confusing "the status of the OperatorGroup could not be determined because the policy is invalid, the status of the Subscription could not be determined because the policy is invalid, the status of the InstallPlan could not be determined because the policy is invalid ..." part. I couldn't find a test that checks for that
TestBuildResources_subInstallPlanApprovalErrorUpdatesStatus test is for that test with message "installPlanApproval is prohibited in spec.subscription" |
e88a03a to
bf8571c
Compare
2f65bf9 to
e3606fb
Compare
9abf6ad to
5789a91
Compare
b536355 to
5a61e84
Compare
fe87bcb to
4421bcf
Compare
37e8685 to
f7febe0
Compare
996ca29 to
b8968c4
Compare
When the subscription section of the OperatorPolicy is invalid, the status might incorrectly indicate that other parts of the policy are invalid. This, combined with the many "... because the policy is invalid" clauses in the overall status, makes it very confusing to read and understand what parts might need to be fixed by the user. Ref: https://issues.redhat.com/browse/ACM-16781 Signed-off-by: yiraeChristineKim <[email protected]>
Signed-off-by: yiraeChristineKim <[email protected]>
Avoid expensive cluster-wide packagemanifest queries, add request timeouts/readiness checks, and handle kubectl failures so bootstrap doesn’t hit TLS handshake timeouts. Signed-off-by: yiraeChristineKim <[email protected]>
b8968c4 to
2679064
Compare
JustinKuli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the persistence!
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
541becd
into
open-cluster-management-io:main
Description:
When the subscription section of the OperatorPolicy is invalid, the status might incorrectly indicate that other parts of the policy are invalid. This, combined with the many "... because the policy is invalid" clauses in the overall status, makes it very confusing to read and understand what parts might need to be fixed by the user.
Change summary
Refactored buildResources to simplify control flow, ensure accurate changed propagation, and improve early error reporting.
Added focused unit tests and dryrun tests; refined e2e case38 to reflect new validation behavior and to fix formatting.
Previous results:
The status message was extremely long, for example:
“NonCompliant; installPlanApproval is prohibited in spec.subscription, the namespace specified in spec.operatorGroup ('example') must match the namespace used for the subscription ('openshift-operators'), the status of the OperatorGroup could not be determined because the policy is invalid, the status of the Subscription could not be determined because the policy is invalid, the status of the InstallPlan could not be determined because the policy is invalid, a relevant installed ClusterServiceVersion could not be found, no CRDs were found for the operator, there are no relevant deployments because the ClusterServiceVersion is missing, the status of the CatalogSource could not be determined because the policy is invalid.”
After the fix:
The only meaningful part is now:
“installPlanApproval is prohibited in spec.subscription.”
Everything else was just unnecessary noise and is no longer included.
This part is actually incorrect: "the namespace specified in spec.operatorGroup ('example') must match the namespace used for the subscription ('openshift-operators'),".
Ref: https://issues.redhat.com/browse/ACM-16781