Remove plan dao methods #766
Conversation
* Currently we save a mapping between names and id and have the data in the spec already. * This was a convince method to quickly get that data, but when moving to CRDs I don't think we should create a resource for a single value mapping
openshift-ci-robot
added
the
size/L
shawn-hurley
added
feature
needs-review
3.10 | release-1.2
|
ACK |
pkg/apb/types.go
Outdated
| return Plan{}, false | ||
| } | ||
|
|
||
| // GetPlanFromID - retrieves a refernece to a plan from a spec by name. Will return |
There was a problem hiding this comment.
typo: refernece -> reference
| return plan, true | ||
| } | ||
| } | ||
| return Plan{}, false |
There was a problem hiding this comment.
Is this a common pattern? Seems kind of weird to me.
There was a problem hiding this comment.
Would it be bad to still return a pointer but then return a copy of the plan?
func (s *Spec) GetPlanFromID(id string) *Plan {
for _, plan := range s.Plans {
if plan.ID == id {
var copy = &Plan{}
*copy = *s.plan
return copy
}
}
return nil
} There was a problem hiding this comment.
It wouldn't be bad as far as I know and if you have strong feelings then I'll change it, but the above behavior is the same behavior you get from the map. val, ok := map["unknownkey"] returns the empty value for val and false.
I would argue that the pointer here does not make sense. We don't want to manipulate the plan in the Spec and therefore a pointer return is misleading IMO.
There was a problem hiding this comment.
Returning-by-value makes sense to me here if we're going to be returning a copy anyway.
* remove plans names -> id mapping from storage. * Currently we save a mapping between names and id and have the data in the spec already. * This was a convince method to quickly get that data, but when moving to CRDs I don't think we should create a resource for a single value mapping * clean up copied comment.
Currently, we save a mapping between names and id and have the data in
the spec already.
This was a convenience method to quickly get that data, but when moving
to CRDs I don't think we should create a resource for a single value
mapping
Changes proposed in this pull request