@mprahl @cqi @jkaluza PTAL

Couldn't get the image with the NVR - parent_brew_build missing => Couldn't directly get the parent image NVR because "parent_brew_build" is missing

What if this is the parent image? Then parent_brew_build will be None and it will cause this to be logged. I think you need to log this only if parent_brew_build is not set and a parent image was found using the old approach.

Depending on how Lightblue works, maybe it's possible only to check for the presence of the parent_brew_build key (not the value). If an old container build doesn't have the key set even when you request the field in your query, then this approach would work.

Edit:
Maybe you could also check if parent_brew_buildis None but layers has more than one entry.

Won't this only contain the child image's immediate parent? You need to find all the parent images of the child.

How do you know the parent image contains the package?

I'm not sure, but I don't think recursive is necessary here. I'd try without it.

log.warning("Couldn't get the image with the NVR - parent_brew_build missing "

My suggestion for this log message is: Couldn't get parent images due to missing child image's parent_brew_build. Fallback to original behavior to get the parent images.

The major thought is the log should tell what didn't succeed and what has to do next to get the parent images. The latter does not involve technical details.

@gnaponie, I think you are right here. You need to check that the parent image returned by the get_images_by_nvrs contains the srpm_name. The get_images_by_nvrs method can almost do that - it accepts srpm_nvr, gets srpm_name from that and uses that as filter in lightblue query. You might just change it slightly to support srpm_name too.

You also need to get the parent image of that parent image and also check if it contains the srpm_name. If it does, you need to check parent of parent of parent image, ... until you a) find the base image which has no other parents or b) find an image which does not contain srpm_name.

The old code does the same, so you can check it for more info :).

@jkaluza @mprahl @cqi @lucarval
Can you please review once again? Thanks.

Please use images=None as the default value (https://pagure.io/freshmaker/issue/457)

Changed.

Optional: How about if srpm_names:?

Could you update the docstring? It only discusses the old approach of querying by the number of layers.

Also, it's nice to document when a function is recursive.

Won't this warning be logged if child_image is a base image? Before logging this, you'll need to verify that this image has more than two layers, which indicates it is not a base image.

Could you keep the comment that was removed explaining this?

It's possible that parent_brew_build is set, but the parent just didn't include the affected SRPM. I'd suggest using that before falling back to this approach. It's guaranteed to be accurate and it'll be faster.

Optional: Instead of backslashes, I find it's cleaner use parentheses. For example:

err = (
    "Cannot find parent of image %s with layer %s "
    "and layer count %d in Lightblue, Lightblue data "
    "is probably incomplete"
    % (children[-1]['brew']['build'], parent_top_layer, parent_build_layers_count)
)

Optional: counter += 1

Some of the strings are unicode, while others aren't. Since this is a Python 3 project, I suggest making them normal strings.

Yeah... I just copied from the previous ones. I'll just change them all.

This was copy pasted from another variable. I'll change in this PR this one, but we'll have to make a separate commit for the other ones.

I'm not sure I understand this comment. Can you explain better?
First it tries to get it with "parent_brew_build". If the field is not there it tries by layer.
Then if no image is found it ends up here.

There is an extra space :)

Could you reword Here described this lates behaviour? I don't quite understand what you mean.

The srpm_names value doesn't look right to me. Isn't child_image['parent_brew_build'] the child image's parent container image and not an SRPM? If I misunderstood, could you please explain?

My point in the last review was that if child_image has parent_brew_build set, it's possible that image is None because the parent image didn't have the SRPM. In this case, you should check to see if parent_brew_build is set, and if so, use get_images_by_nvrs on that value without the SRPM filter.

Also, the first argument to get_images_by_nvrs should be child_image['parent_brew_build'] since image is actually the parent of child_image. It might make it easier to understand if theimage variable somehow mentioned that it was the parent.

From the line if parent is None and children_image_layers_count != 2: below, it insinuates that all base images have two layers, so if len(layers) >= 2: would be True if child_image is a base image. This then causes this warning to be logged which isn't desired.

I would verify this is always the case though.

The point was that the description that follows is the one regarding the latest explained behaviour (by layers).
lates --> latest (typo). I'll try to reword.

Also, the first argument to get_images_by_nvrs should be child_image['parent_brew_build'] since image is actually the parent of child_image. It might make it easier to understand if theimage variable somehow mentioned that it was the parent.

I think you are right. I'm really getting confused by the names... Thinking out loud:
child_image is the current image. child_image['parent_brew_build'] contains the NVR of the parent of the current image. So we need to call get_images_by_nvr to get the actual image object from that NVR.

Mmm this in my mind was another case. But I'm afraid I've confused the logic.
In my mind len(layers) >= 2 emulated the for loop, that skips the first layer for idx, parent_top_layer in enumerate(layers[1:]):, and doesn't refer to if parent is None and children_image_layers_count != 2: which is later, and it wasn't removed/changed...
But I'm afraid this if condition here is wrong, because it wouldn't make sense after the first recursive call.

...or maybe that's ok because layer always remains the same through iterations.

@mprahl Addressed the comments. Could you review once again? Thank you

Optional: None is not needed here.

I find the variable name misleading. "top" implies the last layer, but it's actually the parent layer of child_image.

One way to simplify this method is by removing the counter property. You can do this by shortening the layers property for every recursive call. For instance:

return self.find_parent_images_with_package(image, srpm_name, layers[1:], images)

With this approach, layers is always just the layers of the input child_image. In my opinion, this is a lot easier to read.

Optional: None is not needed here.

Optional: Could you rename this to parent_image? I think it makes it a lot easier to read.

Optional: What do you think about renaming images to child_image_children? It makes it clear what this list is.

This will be incorrectly logged when child_image is a parent image without the specified SRPM, since in this case, image is always None and child_image['parent_brew_build'] is not set.

You can refactor if parent is None and children_image_layers_count != 2: to have one if statement of if children_image_layers_count != 2: that logs this. Then you can nest an additional if statement of if parent is None and log "Cannot find parent..." as you do currently.

I think you need to check if parent_build_layers_count > 1:. layers in this current implementation includes all the layers, even ones that were previously processed higher up in the call stack.

Since the existing logic to detect if something was not a parent image was children_image_layers_count != 2, this would mean the equivalent logic is parent_build_layers_count != 1. This is because children_image_layers_count = parent_build_layers_count + 1.

On second thought, is this log statement even necessary? The similar log statement in the beginning of the method should handle all cases.

@mprahl can you review again? Thank you.

You may want to have an else branch that logs an error since if parent is None, it's due to incomplete lightblue data.

If you want to keep the same behavior as before, you need to unindent these two lines. This is also to make it consistent with the else branch of if parent_image: below, since you search for the parent regardless of the layer count.

I know this behavior preexisted this PR, but I find it strange to resolve all of child_image_children since child_image_children contains images that were individually resolved in previous calls of this method on the call stack.

Optional: It'd be nice consistent with your usage of single quotes vs double quotes. For instance, your patch decorators use single quotes but the method body uses double quotes.

Rather than patching this, it might make sense to patch get_images_by_nvrs so you can verify it was properly called based on the presence of the "parent_brew_build" key.

I find it strange that you're calling find_parent_images_with_package with self.fake_container_images_with_parent_brew_build[0] and having its parent end up being itself.

This unfortunately is inconsistent through the whole project. We should make an effort with future PRs to fix that.

Not sure if there's some issue with the test suite. But this seems like the expected behaviour for similar tests in the same file too. Example I was checking test_parent_images_with_package.

@mprahl I've address the comments again.
I appreciate the reviews, but I've been already asked twice to please speed up the release of this change. So if there aren't big issues with this PR I would move forward and merge it. Can you please review it once more, and in case there's no blocking merge it? For non-blocking change requests we can follow with other PRs.

This comment is now out of date.

:thumbsup: from me, though my comments about the test still stands. Additionally, you're missing a test for when the parent image doesn't include the RPM but is in lightblue. I'll leave it up to you if you want to address that or not.

Made some changes based on the face to face discussion and added some tests.

The first arg must be a list, so [parent_brew_build].

The first arg must be a list, so [parent_brew_build].

What if there are multiple entries in parent_image_builds? Or what if there aren't any as in the case of base images?

It's not clear to me when this is actually done.

:+1: makes sense to me!

Commit 8876bd0 fixes this pull-request