-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support structured policy recommendation results #138
Conversation
6dcfc9f
to
98b352c
Compare
98b352c
to
27041df
Compare
27041df
to
eb24af9
Compare
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
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.
Partial review. Planning to complete it today.
if npReco.Status.State == crdv1alpha1.NPRecommendationStateCompleted { | ||
result, err := r.getRecommendationResult(npReco.Status.SparkApplication) | ||
if err != nil { | ||
klog.Warningf("Failed to get the result for completed NetworkPolicy Recommedation with id %s, error: %v", npReco.Status.SparkApplication, err) |
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.
do you think it might be worth populating ErrorMsg in the CRD status if we fail to fetch the results from CH?
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 updated to populate the ErrorMsg. My only concern is usually we keep consistency between status.State
and status.ErrorMsg
, i.e. status.ErrorMsg
will be empty if the status.State
is not failed. But in the case of failing to fetch the results from CH, we need to choose between
- breaking the consistency, i.e. we may have a State of Completed with some ErrorMsg
- overwriting the State value which indicates the status of the NPR CR
Currently I choose the first option. Please tell me if you have other idea for this.
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.
Just some additional, minor comments.
ab1053b
to
6566be8
Compare
Signed-off-by: Yanjun Zhou <zhouya@vmware.com>
6566be8
to
498af35
Compare
/theia-test-e2e |
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!
This PR adds support for structured policy recommendations with the following changes:
Fix: #134
Signed-off-by: Yanjun Zhou zhouya@vmware.com