pkg/ansible - update status to include failure message on the status. #639
Conversation
shawn-hurley
force-pushed
the
feature/AO-status
branch
from
90ee205
to
c470efe
Compare
| @@ -21,12 +21,14 @@ import ( | |||
| "os" | |||
| "time" | |||
|
|
|||
| ansiblestatus "github.com/operator-framework/operator-sdk/pkg/ansible/controller/status" | |||
There was a problem hiding this comment.
This name is not super great but status seems to be overused.
shawn-hurley
force-pushed
the
feature/AO-status
branch
from
c470efe
to
9a1acbb
Compare
shawn-hurley
changed the title
shawn-hurley
added
the
language/ansible
There was a problem hiding this comment.
I'm interested in thinking about how to re-organize the status module to better use namespaces, so we don't end up doing things like:
status := ansiblestatus.NewAnsibleStatusFromStatusJobEvent(...)
I had to type "ansible" twice and "status" an impressive four times. :)
Also, the difference between a Status and an AnsibleStatus could be a little confusing, and it certainly contributes to the naming ambiguity. Maybe there's another way. But for the moment I'll need to sleep on it and take a second look tomorrow.
| } | ||
|
|
||
| // GetCondition returns the condition with the provided type. | ||
| func GetCondition(status Status, condType ConditionType) *Condition { |
There was a problem hiding this comment.
What do you think of making this, plus Set and Remove below, methods on Status?
There was a problem hiding this comment.
Upstream generally avoids adding methods to the API types, so I think it's generally preferable to use functions. Plus, everything they're messing with is public, so there's not really a reason for it to be a method.
shawn-hurley
force-pushed
the
feature/AO-status
branch
from
9a1acbb
to
4b842d7
Compare
openshift-ci-robot
added
the
size/XL
shawn-hurley
force-pushed
the
feature/AO-status
branch
from
4b842d7
to
93cae11
Compare
shawn-hurley
force-pushed
the
feature/AO-status
branch
from
93cae11
to
6d2b845
Compare
| logrus.Debugf("status was not found") | ||
| u.Object["status"] = map[string]interface{}{} | ||
| statusInterface := u.Object["status"] | ||
| statusMap, _ := statusInterface.(map[string]interface{}) |
There was a problem hiding this comment.
We no longer need to check if Object["status"] exists or is of type map[string]interface{}?
There was a problem hiding this comment.
The below func CreateFromMap will check this and handle the edge cases.
| v1.ConditionTrue, | ||
| ansibleStatus, | ||
| ansiblestatus.SuccessfulReason, | ||
| ansiblestatus.SuccessfulMessage, |
There was a problem hiding this comment.
Would it be useful to record what you did to some degree in the message? Or perhaps you just want that to be in the logs/events?
There was a problem hiding this comment.
I was thinking that events would be more useful for this.
Description of the change:
Updates the ansible operator status to align better conventions
Motivation for the change: