if not image.endswith('-container'):

Can we use brew.package for an exact match?

Consider removing this object. I'm not sure what the purpose of this match is, but it seems to not have any impact.

Do we want to restrict the query to just published content?

Can we use brew.package for an exact match?

If that's the case, consider renaming this method to get_images_by_brew_package.

if not image.endswith('-container'):

You may want to apply this modification as early as possible. This would mean, when the API request is received.

Consider always reaching for the 3rd element. The first one is the git source URL, the second the koji build target, and the third are the options.

What if 3 images are requested, but only 2 are found? We want to raise an error as well right?

The get_nvr parameter is no longer needed as of https://pagure.io/freshmaker/c/d2248617ebe8c574936f6dc7d24a76579e638fd0, see changes in freshmaker/utils.py.

I think we can make this processing a little bit more efficient.

Consider sorting the images in reverse before iterating over them. This will allow skipping a bunch of calls to Koji in most cases.

# Use a dict to map a package to the highest NVR image with matching branch. For example:
#  {"ubi8-container": ContainerImage<nvr=ubi8-container-8.1-100>, "nodejs-12-container": ContainerImage<nvr=nodejs12-container-1.0-101>)}
images_to_rebuild = {}
# Sort images by nvr
images = sorted_by_nvr(images, reverse=True)
for image in images:
  package = ... # get package from image
  if package in images_to_rebuild:
    if image.nvr <= images_to_rebuild[package].nvr:
      # We have already found a better match, skip this image.
      continue
  image_branch = ... # get branch for the image
  if image_branch == expected_branch:
    images_to_rebuild[package] = image

• Can you name the handler class according to filename - RebuildImagesOnAsyncManualBuild?
• I think this handler should be in the ./freshmaker/handlers/koji/ directory, because it uses Koji for rebuilds. Maybe we should put some README in some other PR here to store what has been decided long ago in Proposed solution of https://docs.google.com/document/d/1dx6cuFblaZlf0BZtPU6kv-ZzNJgf2JHWQ-VfSDuHVyM/edit#heading=h.x6yj4z1j38x.

What if 3 images are requested, but only 2 are found? We want to raise an error as well right?

Mmm, yes, I guess. Especially if the requested images are related to each other. Let's change that.

Can we use brew.package for an exact match?

I'm addressing this comment. Because of that, I removed the *, because at this point I believe it should be an exact match. Let me know if you think otherwise.

Can you name the handler class according to filename - RebuildImagesOnAsyncManualBuild?
I think this handler should be in the ./freshmaker/handlers/koji/ directory, because it uses Koji for rebuilds. Maybe we should put some README in some other PR here to store what has been decided long ago in Proposed solution of https://docs.google.com/document/d/1dx6cuFblaZlf0BZtPU6kv-ZzNJgf2JHWQ-VfSDuHVyM/edit#heading=h.x6yj4z1j38x.

I've added some item in the parking lot ideas we have for the guild. If necessary I can file a card.

@lucarval @jkaluza comments addressed. It is ready for another review.

if not image.endswith('-container'):

You may want to apply this modification as early as possible. This would mean, when the API request is received.

@gnaponie, this was not addressed. It would make things much simpler now and in the future if the suffix was added as needed in the initial processing of the API request.

It should be the first one that matches the branch.

Will self.event.dist_git_branch ever be None? If not, then this if statement could be significantly reduced to if self.event.dist_git_branch == git_branch):

This doesn't looks right. Sure the API request has already completed, but we shouldn't set the event as skipped. I think if we raise an exception here, then it should mark the even as failed due to the usage of the fail_event_on_handler_exception decorator.

Images in multi-stream repos may never be tagged with "latest" tag. Instead they will have a per version "latest" tag, e.g. "v3.4", "v3.5". Removing this filter is probably the right thing, but you may get a much larger result set.

This doesn't looks right. Sure the API request has already completed, but we shouldn't set the event as skipped. I think if we raise an exception here, then it should mark the even as failed due to the usage of the fail_event_on_handler_exception decorator.

Yeah we can also mark it as FAILED. I thought SKIPPED maybe was better. But I'm ok also with marking it as FAILED. It seems more consistent maybe with other cases we already mark as FAILED

Will self.event.dist_git_branch ever be None? If not, then this if statement could be significantly reduced to if self.event.dist_git_branch == git_branch):

It is a mandatory field. But we could have build or task_id or git_branch == None, we might have encourred errors retrieving them. So I believe this check would be necessary.

@jkaluza @lucarval it should be complete now. Can you review again?

include_rpms is not a valid parameter

You should use an exact match, not regex.

I would've expected batches to container batches, not images. Should this be:

for batch in batches:
  for image in batch:
    ...

This implies deduplication didn't work as expected.

It looks like the image hasn't been resolved at this point. So this information won't be available.

I modified dev_scripts/find_images_to_rebuild.py to test this out. After modifying the call to _get_default_projection to use the correct parameter include_rpm_manifest, the code executes.

Changing the LightBlue query to not use a regex makes the execution much faster. Less matches to process.

I was able to get it to execute until _record_batches then I hit issues related to the image not being resolved.

Consider using dev_scripts/find_images_to_rebuild.py to continue development on this. Here are the modifications I did:

diff --git a/dev_scripts/find_images_to_rebuild.py b/dev_scripts/find_images_to_rebuild.py
index 8f1f186..11e686d 100755
--- a/dev_scripts/find_images_to_rebuild.py
+++ b/dev_scripts/find_images_to_rebuild.py
@@ -18,8 +18,8 @@ os.environ["REQUESTS_CA_BUNDLE"] = "/etc/ssl/certs/ca-bundle.crt"
 from freshmaker import db, app
 from freshmaker.errata import Errata, ErrataAdvisory
 from freshmaker.events import (
-    ErrataAdvisoryStateChangedEvent, ManualRebuildWithAdvisoryEvent)
-from freshmaker.handlers.koji import RebuildImagesOnRPMAdvisoryChange
+    ErrataAdvisoryStateChangedEvent, ManualRebuildWithAdvisoryEvent, FreshmakerAsyncManualBuildEvent)
+from freshmaker.handlers.koji import RebuildImagesOnRPMAdvisoryChange, RebuildImagesOnAsyncManualBuild
 fedmsg_config = fedmsg.config.load_config()
 dictConfig(fedmsg_config.get('logging', {'version': 1}))
@@ -39,16 +39,25 @@ db.create_all()
 db.session.commit()
 errata = Errata()
-kwargs = {}
-if container_images:
-    EventClass = ManualRebuildWithAdvisoryEvent
-    kwargs['container_images'] = container_images
-else:
-    EventClass = ErrataAdvisoryStateChangedEvent
-event = EventClass(
-    "fake_message", ErrataAdvisory.from_advisory_id(errata, sys.argv[1]),
-    dry_run=True, **kwargs)
-
-handler = RebuildImagesOnRPMAdvisoryChange()
+
+branch = sys.argv[1]
+images = 'etcd'
+kwargs = {
+}
+
+# if container_images:
+#     EventClass = ManualRebuildWithAdvisoryEvent
+#     kwargs['container_images'] = container_images
+# else:
+#     EventClass = ErrataAdvisoryStateChangedEvent
+# event = EventClass(
+#     "fake_message", ErrataAdvisory.from_advisory_id(errata, sys.argv[1]),
+#     dry_run=True, **kwargs)
+
+event = FreshmakerAsyncManualBuildEvent(msg_id='fake-msg', dist_git_branch=branch, container_images=images, dry_run=True)
+
+# handler = RebuildImagesOnRPMAdvisoryChange()
+handler = RebuildImagesOnAsyncManualBuild()
 with patch("freshmaker.consumer.get_global_consumer"):
     handler.handle(event)
diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py
index 79825fd..6d56b6e 100644
--- a/freshmaker/lightblue.py
+++ b/freshmaker/lightblue.py
@@ -1039,7 +1039,7 @@ class LightBlue(object):
                     }
                 ]
             },
-            "projection": self._get_default_projection(include_rpms=False)
+            "projection": self._get_default_projection(include_rpm_manifest=False)
         }
         return self.find_container_images(query)

Executed as: ./find_images_to_rebuild.py extras-rhel-7.8

I used the certs and config from the shared secrets repo.

@lucarval thank you for the review! I've addressed all the comments and uploaded a script similar to your patch (I did change a couple of things, like the fact that the parameter should be a list and the name should contain "-container" because the check gets done by the earlier in the API) that works fine on my machine.

@gnaponie, nice! When I ran your script it does complete successfully. I did notice that the dep_on_id attribute for the etcd-container is null. Should this be pointing to the rhel-server-container build?

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|rhel-server-container|
2|etcd-container|

I think the output should've been:

1|rhel-server-container|
2|etcd-container|1

Debugging _record_batches I can see that it does create two batches as expected. So maybe the dependency between builds is just not being recorded properly.

@lucarval there were a couple of lines missing. It should be there now.

Added a commit for some unrelated flake8 errors that suddenly started failing.

Let's remove this hard coded list and accept a list of images as a parameter.

Oh! Actually, just remove this line and use container_images from above :)

@gnaponie, if I run this with branch rhel-8.2.0 and images ubi8-container and python-38-container, then I get the following:

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

This is exactly what's expected! Yay!

Now, if I run with branch rhel-8.2.0 and just the image python-38-container, then I get the same output:

🐚  sqlite3 freshmaker.db 'SELECT `id`, `name`, `dep_on_id` FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

Is that expected? Shouldn't just python-38-container be built?

```

It seems quite odd that this check is buried here. What scenario would trigger this anyways?

BTW, I think the logging logic is not quite right, I'm seeing this in my logs:

Found container images to rebuild in following order:
   Batch 0:
      - containers/ubi8#2f70d2e5b96ea67813c0f06b74a23775a118e7db (based on [None])
      - containers/s2i-core#9f55e017a7daebc2b3aeb7538863b2ad7923a8ff (based on None)
      - containers/s2i-base#f2c337e3945859c323e03acf857f7d397766ec81 (based on None)
      - containers/python-38#46ca3db6ca6d20b02618cde32142dcc98965e45d (based on None)

I think the order is correct, but each one should be built as a separate batch since each image is based on the previous image in the list.

Suggestion: Remove this method completely, and just use freshmaker-cli to get this sort of information if needed.

It would be useful to list which images are problematic.

missing_images = set(self.event.container_images) - set(images_to_rebuild.keys())
if missing_images:
   ...

@gnaponie, if I run this with branch rhel-8.2.0 and images ubi8-container and python-38-container, then I get the following:
🐚 sqlite3 freshmaker.db 'SELECT id, name, dep_on_id FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

This is exactly what's expected! Yay!
Now, if I run with branch rhel-8.2.0 and just the image python-38-container, then I get the same output:
🐚 sqlite3 freshmaker.db 'SELECT id, name, dep_on_id FROM artifact_builds;'
1|ubi8-container|
2|s2i-core-container|1
3|s2i-base-container|2
4|python-38-container|3

Is that expected? Shouldn't just python-38-container be built?
```

I think you are right. I removed some lines (it was about directly building that one image if only that one was requested), I don't recall now why I removed it.... I'll put it back.

It seems quite odd that this check is buried here. What scenario would trigger this anyways?
BTW, I think the logging logic is not quite right, I'm seeing this in my logs:
Found container images to rebuild in following order:
Batch 0:
- containers/ubi8#2f70d2e5b96ea67813c0f06b74a23775a118e7db (based on [None])
- containers/s2i-core#9f55e017a7daebc2b3aeb7538863b2ad7923a8ff (based on None)
- containers/s2i-base#f2c337e3945859c323e03acf857f7d397766ec81 (based on None)
- containers/python-38#46ca3db6ca6d20b02618cde32142dcc98965e45d (based on None)

I think the order is correct, but each one should be built as a separate batch since each image is based on the previous image in the list.
Suggestion: Remove this method completely, and just use freshmaker-cli to get this sort of information if needed.

Yeah, this is here only for debugging I'd say, it's the same method we have in other handles. Perhaps it is a good idea to remove it completely.

@lucarval I hope this is the last time :'(

@lucarval I believe (if I understood correctly what this parameter is for) that this should be enough to address the brew_target parameter.

This should be 3. I only see at most sys.argv[2] being used. The example usage docstring also needs updating.

This doesn't seem to be used.

No need to use a f-string here. Just use name directly: "rvalue": name

I left a couple of very minor comments. This lgtm once those are addressed :thumbsup:

Comments addressed. @lucarval I'll just double check with you in chat, if that's ok, I'd merge it.

:+1: Merge away!