Bug 1566924 - renaming crd resources #907
Conversation
shawn-hurley
requested review from
eriknelson,
jmrodri and
rthallisey
and removed request for
eriknelson and
jmrodri
openshift-ci-robot
added
the
size/XXL
shawn-hurley
added
the
do-not-merge/hold
shawn-hurley
added
bug
needs-review
and removed
do-not-merge/hold
|
@jmrodri @eriknelson This appears to be failing due to config changes between automationbroker-apb and the image created from this PR. Feels like the first time that the APB is hitting this problem. |
pkg/dao/crd/dao.go
Outdated
| } | ||
|
|
||
| // GetBindInstance - Retrieve a specific bind instance from the kvp API | ||
| func (d *Dao) GetBindInstance(id string) (*apb.BindInstance, error) { | ||
| log.Debugf("get binding instance: %v", id) | ||
| bi, err := d.client.ServiceBindings(d.namespace).Get(id, metav1.GetOptions{}) | ||
| log.Debugf("get binidng instance: %v", id) |
pkg/dao/crd/dao.go
Outdated
| } else if d.IsNotFoundError(err) { | ||
| si, err := d.client.BundleInstances(d.namespace).Get(id, metav1.GetOptions{}) | ||
| if err != nil { | ||
| log.Errorf("Unable to update the job state: %v - %v", token, err) |
There was a problem hiding this comment.
I think the language on the errors in this else block should be "Unable to get the job state", right? This is a getter?
There was a problem hiding this comment.
Also, err != nil and si.Status.Jobs branches here look the same? Could combine them?
There was a problem hiding this comment.
Is this the BundleInstance not found case?
pkg/dao/crd/dao.go
Outdated
| return d.GetState("", key) | ||
| bi, err := d.client.BundleBindings(d.namespace).Get(key, metav1.GetOptions{}) | ||
| if err != nil { | ||
| log.Errorf("Unable to update the job state: %v - %v", key, err) |
There was a problem hiding this comment.
Logs should read not found?
| } | ||
|
|
||
| // GetStateByKey - Retrieve a job state from the kvp API for a job key | ||
| func (d *Dao) GetStateByKey(key string) (apb.JobState, error) { | ||
| return d.GetState("", key) | ||
| bi, err := d.client.BundleBindings(d.namespace).Get(key, metav1.GetOptions{}) |
There was a problem hiding this comment.
Why does GetStateByKey only care about bindings? The usage in the broker only cares about that case?
There was a problem hiding this comment.
Yes that is the case, the only thing that has a key is the binding as well.
pkg/dao/crd/dao.go
Outdated
| return apb.JobState{}, err | ||
| } | ||
| for token, j := range bi.Status.Jobs { | ||
| // Assuming a single bind job happens per binding instance. |
There was a problem hiding this comment.
👍 seems like a fair assumption.
| Status: metav1.StatusFailure, | ||
| Code: http.StatusNotFound, | ||
| Reason: metav1.StatusReasonNotFound, | ||
| }} | ||
| } | ||
|
|
||
| // FindJobStateByState - Retrieve all the jobs that match the specified state | ||
| func (d *Dao) FindJobStateByState(state apb.State) ([]apb.RecoverStatus, error) { |
| } | ||
| return rs, nil | ||
| return rss, nil | ||
| } | ||
|
|
||
| // GetSvcInstJobsByState - Lookup all jobs of a given state for a specific instance | ||
| func (d *Dao) GetSvcInstJobsByState(ID string, state apb.State) ([]apb.JobState, error) { |
There was a problem hiding this comment.
It's a little confusing seeing a "get service instance jobs by state" that also is looking for bindings? Does this imply that I can pass this a binding ID and it will give me back relevant jobs?
There was a problem hiding this comment.
Yes that is the case. I am making the functionality equivalent here but I agree with your point. I think this interface could use a second pair of eyes.
There was a problem hiding this comment.
I'm ACKing this despite the CI issues because their root cause is understood and unrelated, and I've pulled this PR and confirmed it's working as expected.
@shawn-hurley just hit those log statements as discussed and I'm g2g.
pkg/broker/broker.go
Outdated
| @@ -1466,6 +1466,9 @@ func (a AnsibleBroker) LastOperation(instanceUUID uuid.UUID, req *LastOperationR | |||
|
|
|||
| jobstate, err := a.dao.GetState(instanceUUID.String(), req.Operation) | |||
| if err != nil { | |||
| if a.dao.IsNotFoundError(err) { | |||
| return &LastOperationResponse{}, ErrorNotFound | |||
| } | |||
| // not sure what we do with the error if we can't find the state | |||
| log.Error(fmt.Sprintf("problem reading job state: [%s]. error: [%v]", instanceUUID, err.Error())) | |||
There was a problem hiding this comment.
Should we move the log statement to the top? or do we not want to log this error if we can't find it?
| return err | ||
| } | ||
|
|
||
| func (d *Dao) updateJobState(js *v1.JobState, spec v1.JobStateSpec, state apb.JobState) error { |
There was a problem hiding this comment.
Hey this code looks familiar. Glad I did this :)
Describe what this PR does and why we need it:
Changes proposed in this pull request
Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
Which issue this PR fixes (This will close that issue when PR gets merged)
fixes <issue_number>