[3.5] Bug 1422376: Fix resolving ImageStreamImage latest tag #13090
Conversation
mfojtik
changed the title
pkg/image/api/helper.go
Outdated
| if event.Image != imageID { | ||
| continue | ||
| if d, err := digest.ParseDigest(event.Image); err == nil { | ||
| if !strings.HasPrefix(d.Hex(), imageID) && !strings.HasPrefix(event.Image, imageID) { |
There was a problem hiding this comment.
we use this same digest and event.Image logic (and the item below) in the ResolveImageId in this same file. Time for a helper method?
There was a problem hiding this comment.
not sure if helper is possible... those two seems to have inverted logic.
There was a problem hiding this comment.
that's what the ! is for 😄 DigestOrImageMatch() vs !DigestOrImageMatch()
There was a problem hiding this comment.
ok, I gotcha and I was wrong :-)
pkg/image/api/helper.go
Outdated
| @@ -642,8 +648,10 @@ func ResolveLatestTaggedImage(stream *ImageStream, tag string) (string, bool) { | |||
| func DockerImageReferenceForImage(stream *ImageStream, imageID string) (string, bool) { | |||
| tag, event := LatestImageTagEvent(stream, imageID) | |||
| if len(tag) == 0 { | |||
| glog.Infof("no tag") | |||
There was a problem hiding this comment.
are these two log statements useful? Print out the stream and imageID maybe?
There was a problem hiding this comment.
also, are they Info level or debugging level type?
There was a problem hiding this comment.
sry, debugging left over.
|
also per our retro discussion...where dem tests at? |
|
@pweil- there is a |
mfojtik
force-pushed
the
fix_latest_image_tagevent_35
branch
2 times, most recently
from
a2bf080 to
df5d176
Compare
|
@pweil- now with test and helper :-) |
|
[test] |
|
LGTM |
pkg/image/api/helper.go
Outdated
| continue | ||
| } | ||
| if latestTagEvent == nil || (latestTagEvent != nil && event.Created.After(latestTagEvent.Created.Time)) { | ||
| if digestOrImageMatch(event.Image, imageID) && (latestTagEvent == nil || (latestTagEvent != nil && event.Created.After(latestTagEvent.Created.Time))) { |
There was a problem hiding this comment.
This condition freaks me out 😨
There was a problem hiding this comment.
could be reduced to this, right?
if digestOrImageMatch(event.Image, imageID) &&
(latestTagEvent == nil || event.Created.After(latestTagEvent.Created.Time))
There was a problem hiding this comment.
yeah, i'm a bracket guy...
There was a problem hiding this comment.
@pweil- if latestTagEvent is nil, then the right part will panic(), or i'm drunk?
mfojtik
force-pushed
the
fix_latest_image_tagevent_35
branch
from
df5d176 to
232330d
Compare
|
[merge] |
|
Evaluated for origin test up to 232330d |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/520/) (Image: devenv-rhel7_5970) |
|
Evaluated for origin merge up to 232330d |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/520/) (Base Commit: 4a74666) |
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1422376
Master: #13089