Bug 1576881 - Add /v2/_catalog support for openshift adapter #74
Conversation
|
@johnkim76 Do we have a bug for this? If so I believe we need to create 2 PRs... one against master and one against release-1.0 |
There was a problem hiding this comment.
Use *f versions of logging statements when substitution strings are used. Also add some more logging for the error conditions I noted, particularly the one where we get a !200 status code. Definitely want to print the actual status code we got to make it easier to debug in the future.
| images := r.Config.Images | ||
| log.Debugf("Configured to use images: %v", images) | ||
| if r.Config.Images != nil { | ||
| log.Debug("Configured to use images: %v", r.Config.Images) |
| log.Debug("Configured to use images: %v", r.Config.Images) | ||
| return r.Config.Images, nil | ||
| } | ||
| log.Debug("Did not find images in config, attempting to discover from %s/v2/_catalog", r.Config.URL) |
There was a problem hiding this comment.
Use log.Debugf whenever you have a substitution string in the string. i.e. %s or %v.
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != 200 { | ||
| return nil, errors.New(resp.Status) |
There was a problem hiding this comment.
I would add an error or warning log here. It would be nice to get as much information as to why we hit this exceptional case. I just know when we hit this we're going to wonder what really happened, and we'll add a log debug in to figure it out. If the warning or error were there already, we'd know why we got a !200.
|
Thanks for the feedback @jmrodri. Made the requested changes. |
dymurray
changed the title
|
Please look through the changes on #77 and if you agree with the name change I will make the same changes here for master. |
dymurray
dismissed
jmrodri’s stale review
Old review... Gone through a lot of changes since then :)
| } | ||
| log.Debugf("Did not find images in config, attempting to discover from %s/v2/_catalog", r.Config.URL) | ||
|
|
||
| req, err := http.NewRequest("GET", fmt.Sprintf(partnerCatalogURL, r.Config.URL), nil) |
There was a problem hiding this comment.
It looks like @maleck13's change only affects the loadSpec function not the Catalog call... this is unique to the partner adapter.
I'm fine with making the change to loadSpec once it's merged.
No description provided.