Update RHCC registry with new query and logic to handle Spec creation #158
Conversation
pkg/apb/rhcc_registry.go
Outdated
|
|
||
| err = json.Unmarshal([]byte(hist.History[0]["v1Compatibility"]), &conf) | ||
| if err != nil { | ||
| r.log.Error("Error unmarshalling intermediary JSON response") |
There was a problem hiding this comment.
Can we print the error out here? I think that helps while debugging if something goes wrong
pkg/apb/rhcc_registry.go
Outdated
|
|
||
| r.log.Debug(r.config.Url) | ||
| encodedSpec := conf.Config.Label.Spec | ||
| decodedSpecYaml, _err := b64.StdEncoding.DecodeString(encodedSpec) |
There was a problem hiding this comment.
I would not create a new error variable here, I think that it should be err, imo
|
|
||
| hist := struct { | ||
| History []map[string]string `json:"history"` | ||
| }{} |
There was a problem hiding this comment.
Why are these all private structs defined within the method? I would've made them normal structs at the top of the file.
There was a problem hiding this comment.
I made these private structs because they are intermediary objects I need to parse the JSON down to the spec which gives us the labels. I figured this data object wouldn't be used outside of this function.
There was a problem hiding this comment.
I'm fine with leaving everything in this one file. I'd rather we not touch anything outside since this will change post TP.
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| type label struct { |
There was a problem hiding this comment.
Part of this is in pkg/apb/types/go https://github.com/openshift/ansible-service-broker/blob/master/pkg/apb/types.go#L15.
The way you're reading in the json IMO is better. Right now we use the string 'com.redhat.apb.spec' as a map key when we should really be decoding into a struct. We could either leave this as additional tech debt or try and fix it now. I might take a crack at doing this tomorrow.
There was a problem hiding this comment.
Ah I see, I'd be ok with it being in the struct instead of being a map key. That makes sense to me. 👍
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| type label struct { |
There was a problem hiding this comment.
Ah I see, I'd be ok with it being in the struct instead of being a map key. That makes sense to me. 👍
|
Update here: After some testing it looks like RHCC images can be inspected. This change may just be a config file change to point at the correct registry. |
|
Above issue is used to track Ryan's comment that there is overlapping logic here we can make a lot cleaner for using different registries. |
dymurray
changed the title
There was a problem hiding this comment.
@dymurray I'll leave it to you to test. Just give me a 👍 once tested since I'm having trouble connecting to the registry.
|
|
||
| hist := struct { | ||
| History []map[string]string `json:"history"` | ||
| }{} |
There was a problem hiding this comment.
I'm fine with leaving everything in this one file. I'd rather we not touch anything outside since this will change post TP.
Updated config to use URL too error cleanup
dymurray
dismissed
jmrodri’s stale review
Out of date, worked your suggestions in but currently on PTO.
|
ACK |
…openshift#158) * Update RHCC registry with new query and logic to handle Spec creation Updated config to use URL too error cleanup * Send Metadata field when creating a new service class for the Service Catalog
No description provided.