redhat-exd-rebuilds / freshmaker

Freshmaker is a service that automatically rebuilds content.
https://redhat-exd-rebuilds.github.io/freshmaker/
MIT License
9 stars 23 forks source link

Replace the parent image only with the latest one #252

Closed gnaponie closed 1 year ago

gnaponie commented 1 year ago

Due to regressing content, it might happen, in rare conditions, that the parent image of the image we are rebuilding after a CVE fix doesn't include all the latest fixes.

Example: we have 3 images: NVR | CVE fixes foo-1-125: fix-a, fix-b foo-1-124: fix-a, fix-c bar-2-220: fix-a, fix-b foo is parent of bar. Before this change, Freshmaker would ask Pyxis if there's already a parent image fixing fix-c, and it would return foo-1-124, so that Freshmaker can use that as parent of bar. But that image is missing fix-b! To prevent this issue, from now on, Freshmaker ask to Pyxis only the latest image.

Tests are not updated, since the current tests should cover the case.

gnaponie commented 1 year ago

@qixiang @FernandesMF PTAL Please notice that I didn't add any new tests, since the existing one is testing the function already. I thought about one that would test this corner case, but since we are mocking the Pyxis data nothing came to my mind. In case you have any suggestions I'll be happy to add some more tests. Thank you.

gnaponie commented 1 year ago

@FernandesMF ok, thanks for the heads-up. I think I'll rebase on main to include your changes, and if it keeps failing I'll add a commit to fix the black check.

gnaponie commented 1 year ago

@qixiang @FernandesMF could you please check again? I've fixed black, but I'm not really sure why it still fails. I've applied all the suggestions it prompted.

FernandesMF commented 1 year ago

Hi @gnaponie! I pulled the code from your branch and took a look at black. It turns out black is acting weird indeed.

When I ran black, it complained about line 61 in test_pyxis_gql_async.py,

        fake_query = ds.Query.find_repositories(page=0, page_size=2, filter={},).select(

and wanted to rewrite it the way you had it before with respect to the arguments of find_repositories, with the arguments spread one per line.

Seems like we found a bug, and black is not idempotent in this case.

I found a quick solution of disabling it for this line. You can put a # fmt: skip by the end of the select block:

        fake_query = ds.Query.find_repositories(page=0, page_size=2, filter={},).select(
           ...
        )  # fmt: skip

at least, that did the trick in my test. Notice the two spaces before the #!

FernandesMF commented 1 year ago

Oh, and have to manually squash all our commits together in Freshmaker. So don't forget to squash all your commits when you are ready to merge!

qixiang commented 1 year ago

Hi @gnaponie! I pulled the code from your branch and took a look at black. It turns out black is acting weird indeed.

If you're running the same version black (as it is in the github tests) locally, it should show you consistent result. This is caused by a change in black version 23 (https://github.com/psf/black/blob/ddfecf06c13dd86205c851e340124e325ed82c5c/CHANGES.md?plain=1#L323).

It's unnecessary to have the magic trailing comma in:

fake_query = ds.Query.find_repositories(page=0, page_size=2, filter={},).select(

so we can just remove it:

fake_query = ds.Query.find_repositories(page=0, page_size=2, filter={}).select(

The rest of the code looks good to me.