Adding ability to warn and filter out bad specs. #571
Conversation
openshift-ci-robot
added
the
size/L
|
Changes Unknown when pulling 6fc5faf on shawn-hurley:issue-388 into ** on openshift:master**. |
pkg/broker/broker.go
Outdated
| services[i] = SpecToService(spec) | ||
| ser, err := SpecToService(spec) | ||
| if err != nil { | ||
| a.log.Debugf("no adding spec to list of services due to error transforming to service - %v", err) |
|
Changes Unknown when pulling ed476c8 on shawn-hurley:issue-388 into ** on openshift:master**. |
| service := SpecToService(&spec) | ||
| service, err := SpecToService(&spec) | ||
| if err != nil { | ||
| a.log.Debugf("spec was not added due to issue with transformation to serivce - %v", err) |
There was a problem hiding this comment.
do we intend to log an error as DEBUG? seems odd.
There was a problem hiding this comment.
I think that warning might be better. This is an APB gave us bad data which we have not treated as an error before I don't think it should be Errorf IMO
| } | ||
| return []schema.PrimitiveType{schema.UnspecifiedType} | ||
| return nil, fmt.Errorf("Could not find the parameter type for: %v", paramType) |
There was a problem hiding this comment.
so what sparked this patch originally?
There was a problem hiding this comment.
#388 sparked this patch.
The basics of the issue are that the UnspecifiedType does not marshall into json, therefore the only things I can think to do are:
-
either catch this by converting all
specstoservicesduring load time of the specs -
When
/catalogis called and we convertspecstoserviceswe catch that error/unspecified type and do not add it to the list ofservicesbut do not blow up the entire response. -
blow up the entire response w/ a 500.
I choose to go down this route because if we can not gracefully handle the Unspecified type in the response then I think we should treat that as an error.
| for _, pd := range params { | ||
| k := pd.Name | ||
|
|
||
| t, err := getType(pd.Type) |
There was a problem hiding this comment.
Why not just check to see if the type is UnspecifiedType? I'm not sure I like the error on the getType. Is it really an error on getType? It looked for one, didn't fine a good one, said it was unspecified.
t := getType(pd.Type)
if t == []schema.PrimitiveType{schema.UnspecifiedType} {
return properties
}
There was a problem hiding this comment.
I think it is reasonable that an ununderstandable type is an error. Especially since we can not use that type for it's intended purpose. If you feel strongly though, I can change the initial error here instead of in getType function.
Describe what this PR does and why we need it:
Will add debug messages when a type from an APB parameter is not defined, and remove from the services for catalog.
Changes proposed in this pull request
Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #388