This needs a good amount of work. It's mostly for illustration purposes.

I tested this against one of the latest CVEs, and it deduplicated 5 additional images out of 213.

Is this published=None correct? Step 5 mentions to check "if the “original_nvr” image is published". Should it just keep the default value True?

Is this published=None correct? Step 5 mentions to check "if the “original_nvr” image is published". Should it just keep the default value True?

Right, but step 6 says "If not published, use the “original_nvr” as the nvr and jump back to step 1". By using published=None, we get the image regardless of whether or not it's been published. Then, we decide in the code how to treat it. Otherwise, we'd have to make two queries to Lightblue, published=True and published=False, which seems unnecessary.

@gnaponie, @jkaluza, do we have an existing mechanism for querying Freshmaker from itself like this? From what I can see, lightblue.py does not use a db connection, nor it knows how to access Freshmaker's API.

I see a couple of options:

1. Teach lightblue.py to access Freshmaker's API via HTTP requests.
2. Pass in the db connection when creating the LightBlue object from freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py.

I think, ideally, we'd only have the web API interacting with the database, which would make option 1 more favorable. However, this is definitely not the case right now. Both the backend/consumers and the web API interact directly with the database. Option 2 seems to be more inline with the current architecture.

Thoughts?

@gnaponie, @jkaluza, do we have an existing mechanism for querying Freshmaker from itself like this? From what I can see, lightblue.py does not use a db connection, nor it knows how to access Freshmaker's API.
I see a couple of options:

Teach lightblue.py to access Freshmaker's API via HTTP requests.
Pass in the db connection when creating the LightBlue object from freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py.

I think, ideally, we'd only have the web API interacting with the database, which would make option 1 more favorable. However, this is definitely not the case right now. Both the backend/consumers and the web API interact directly with the database. Option 2 seems to be more inline with the current architecture.
Thoughts?

I've taken the approach of reaching the DB directly.

@gnaponie, @jkaluza, do we have an existing mechanism for querying Freshmaker from itself like this? From what I can see, lightblue.py does not use a db connection, nor it knows how to access Freshmaker's API.
I see a couple of options:
Teach lightblue.py to access Freshmaker's API via HTTP requests.
Pass in the db connection when creating the LightBlue object from freshmaker/handlers/koji/rebuild_images_on_rpm_advisory_change.py.
I think, ideally, we'd only have the web API interacting with the database, which would make option 1 more favorable. However, this is definitely not the case right now. Both the backend/consumers and the web API interact directly with the database. Option 2 seems to be more inline with the current architecture.
Thoughts?

I've taken the approach of reaching the DB directly.

I'm gonna rethink this a bit. I think we can use a simple heuristic to simply strip off the .xxxx suffix from the release value, and use the modified NVR to query Lightblue. This would remove the dependency of having to query the Freshmaker API and should make things simpler.

I've finally understood what this PR is about :). It makes sense to me. It surprises me a bit that the image built by Freshmaker is not included in the repository, but I believe you that's the case :).

@lucarval Could you please resolve the conflicts here? Thank you.

Maybe these two lines could be simplified like {python_image.nvr, nodejs_image.nvr}.

A few test case classes are written based on ModelsTestCase which already has the code to create and drop db separated into setUp and tearDown. With autouse, does this mock_db also affect those tests as well? Looks like we can drop the same code from the setUp and tearDown:

    def setUp(self):
        super(ModelsTestCase, self).setUp()
        db.session.remove()
        db.drop_all()
        db.create_all()
        db.session.commit()
        self.user = User(username='tester1')
        db.session.add(self.user)
        db.session.commit()
    def tearDown(self):
        super(ModelsTestCase, self).tearDown()
        db.session.remove()
        db.drop_all()
        db.session.commit()

@lucarval what's the status of this PR?

@lucarval what's the status of this PR?

It's outdated. It needs to be reworked to no longer require access to the DB. I just haven't had the bandwidth. If there's urgency in getting this done, I suggest someone else picks it up. Otherwise, I'll try to have it cleaned up this week.

Maybe these two lines could be simplified like {python_image.nvr, nodejs_image.nvr}.

Good idea! Done.

@cqi, I've reworked the changes to be more efficient and not require additional DB queries. This also simplified testing quite a bit.

I tested these changes by using dev_scripts/find_images_to_rebuild.py 53625. The diff with and without my changes are:

diff <(< master.json jq '.[].original_nvr' -r | sort) <(< modified.json jq '.[].original_nvr' -r | sort)
55d54
< s2i-base-container-1-131.1584463498

As you can see, with my changes, we wouldn't rebuild s2i-base-container image twice s2i-base-container-1-131.1584463498 and s2i-base-container-1-141.1584463496. They are properly deduplicated.

@lucarval there's no urgency, but I was trying to close old PRs and this was opened 2 months ago.
Let us know when this is ready for another review. Thank you.

We could just sorted(r["repository"] for r in repositories)

Since freshmaker is already Python 3 only, perhaps we could using f-strings to format the string instead. IMO, it could make code more compact.

LGTM. Leave two minor suggestions.

Additionally, get_registry_repositories will be called recursively through the for-loop of for parent_id, image in enumerate(images):, probably it could be tested to see if it would have potential performance issue based on the known cases of image builds and whether it is worth to convert the for-loop into a multi-threaded solution.

@cqi, I've incorporated your suggestion to change to use sorted(r["repository"] for r in repositories)

I've decided that using f-strings wouldn't improve things by much, so I left it as is.

Regarding the performance concern, the check for a . on the release value pretty much narrows it down to only the cases we should always check. I've ran this against a recent advisory and didn't notice a significant increase in computation time.

:thumbsup:

+1 on the new commit,
I trust you both for the other one :)