-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1507111 - Do not force image tag to be IP + Port #540
Conversation
r.Config.Namespaces = append(r.Config.Namespaces, "openshift") | ||
} | ||
spec.Image = image.Name | ||
namespace := strings.Split(image.Name, "/")[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like blindly accessing index of an array
ns_list := strings.Split(image.Name, "/") | ||
var namespace string | ||
if len(ns_list) == 0 { | ||
r.Log.Errorf("Image [%v] is not in the proper format. Erroring.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing image name in print statement.
} | ||
} | ||
if strings.HasPrefix(image.Name, registryIP) == false { | ||
r.Log.Debugf("Image does not have a registry IP as prefix. This might cause problems but not erroring out.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not Warningf
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djzager Because if you look a few lines below it is possible that S2I images will not have any registry prefix at all. I'm indifferent to which type of logging statement it is but with 10 S2I images that would output 10 warning statements in the logs for something that is expected behavior.
Feels weird to warn the user over expected behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured there was a reason. In this case, the text of the message is the warning. Works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visual ACK
* Bug 1507111 - Do not force image tag to be IP + Port * Better warning statement * Better error handling for bad image names * Forgot print statement * Linting fix
This PR changes default behavior of local registry adapter to not error out if APB name does not start with IP+port. This was erroring out when a user's image was being tagged with the svc name instead.
Changes proposed in this pull request
openshift
if not set