freshmaker

Clone

#483 Override parent image
Merged 4 years ago by gnaponie. Opened 4 years ago by gnaponie.
gnaponie/freshmaker FACTORY-5960  into  master

file modified
+36 -12 
@@ -1247,6 +1247,33 @@ 
 

  

          return latest_parent
 

  

+     def find_parent_image_from_child(self, child_image):
 

+         """
 

+         Returns the parent of the input image. If the parent is not found it returns None.
 

+ 

+         :param ContainerImage child_image: ContainerImage object, image for which we need to find the parent.
 

+ 

+         :return: parent of the input image.
 

+         :rtype: ContainerImage object
 

+ 

+         """
 

+         parent_brew_build = child_image.get("parent_brew_build")
 

+         if parent_brew_build:
 

+             return parent_brew_build
 

+         # We need to resolve the image in here because "parent_image_builds" needs to be there
 

+         # and it gets populated when the image gets resolved.
 

+         child_image.resolve_commit()
 

+         # If the parent is not in `parent_brew_build` we can try to look for the parent in Brew,
 

+         # using the field `parent_image_builds` (searching for the nvr), which should always be there.
 

+         # In case parent_brew_build is None and child_image["parent_image_builds"] == {},
 

+         # it means we found a base image and there's no parent image.
 

+         if child_image["parent_image_builds"]:
 

+             parent_brew_build = [
 

+                 i["nvr"] for i in child_image["parent_image_builds"].values()
 

+                 if i["id"] == child_image["parent_build_id"]][0]
 

+ 

+         return parent_brew_build
 

+ 

      def find_parent_images_with_package(self, child_image, srpm_name, images=None):
 

          """
 

          Returns the chain of all parent images of the image which contain the
 
@@ -1264,18 +1291,7 @@ 
 

  

          children = images if images else [child_image]
 

          # We first try to find the parent from the `parent_brew_build` field in Lightblue.
 

-         parent_brew_build = child_image.get("parent_brew_build")
 

-         # We need to resolve the image in here because "parent_image_builds" needs to be there
 

-         # and it gets populated when the image gets resolved.
 

-         child_image.resolve(self, children)
 

-         # If the parent is not in `parent_brew_build` we can try to look for the parent in Brew,
 

-         # using the field `parent_image_builds` (searching for the nvr), which should always be there.
 

-         # In case parent_brew_build is None and child_image["parent_image_builds"] == {},
 

-         # it means we found a base image, so we'll just continue and return the children.
 

-         if not parent_brew_build and child_image["parent_image_builds"]:
 

-             parent_brew_build = [
 

-                 i["nvr"] for i in child_image["parent_image_builds"].values()
 

-                 if i["id"] == child_image["parent_build_id"]][0]
 

+         parent_brew_build = self.find_parent_image_from_child(child_image)
 

          # We've reached the base image, stop recursion
 

          if not parent_brew_build:
 

              return children
 
@@ -1684,6 +1700,14 @@ 
 

                      image, srpm_name, [])
 

                  if rebuild_list[srpm_name]:
 

                      image['parent'] = rebuild_list[srpm_name][0]
 

+                 else:
 

+                     parent_brew_build = self.find_parent_image_from_child(image)
 

+                     if parent_brew_build:
 

+                         parent = self.get_images_by_nvrs([parent_brew_build], published=None)
 

+                         if parent:
 

+                             parent = parent[0]
 

+                             parent.resolve(self, images)
 

+                             image['parent'] = parent
 

                  rebuild_list[srpm_name].insert(0, image)
 

              return rebuild_list

file modified
+36 
@@ -1624,6 +1624,42 @@ 
 

              "Couldn't find parent image some-original-nvr-7.6-252.1561619826. "
 

              "Lightblue data is probably incomplete"))
 

  

+     @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs")
 

+     @patch('freshmaker.lightblue.LightBlue.find_images_with_packages_from_content_set')
 

+     @patch('freshmaker.lightblue.LightBlue.find_unpublished_image_for_build')
 

+     @patch('freshmaker.lightblue.LightBlue.find_parent_images_with_package')
 

+     @patch('os.path.exists')
 

+     def test_parent_images_with_package_using_field_parent_brew_build_parent_empty(
 

+             self, exists, find_parent_images_with_package, find_unpublished_image_for_build,
 

+             find_images_with_packages_from_content_set, cont_images):
 

+         exists.return_value = True
 

+ 

+         image_a = ContainerImage.create({
 

+             "brew": {"package": "image-a", "build": "image-a-v-r1"},
 

+             "parent_brew_build": "some-original-nvr-7.6-252.1561619826",
 

+             "repository": "repo-1",
 

+             "commit": "image-a-commit",
 

+             "repositories": [{"repository": "foo/bar"}],
 

+             "rpm_manifest": [{
 

+                 "rpms": [
 

+                     {"srpm_name": "dummy"}
 

+                 ]
 

+             }]
 

+         })
 

+ 

+         find_parent_images_with_package.return_value = []
 

+         find_unpublished_image_for_build.return_value = image_a
 

+         find_images_with_packages_from_content_set.return_value = [image_a]
 

+         cont_images.side_effect = [self.fake_container_images_with_parent_brew_build, [], []]
 

+ 

+         lb = LightBlue(server_url=self.fake_server_url,
 

+                        cert=self.fake_cert_file,
 

+                        private_key=self.fake_private_key)
 

+         ret = lb.find_images_to_rebuild(["dummy-1-1"], ["dummy"])
 

+ 

+         self.assertEqual(len(ret), 1)
 

+         self.assertIsNotNone(ret[0][0].get("parent"))
 

+ 

      @patch("freshmaker.lightblue.ContainerImage.resolve_published")
 

      @patch("freshmaker.lightblue.LightBlue.get_images_by_nvrs")
 

      @patch("os.path.exists")

Freshmaker should always override the parent image (that is in the
Dockerfile) when triggering the buildContainer task. This was not
happening. In some special cases the parent remained empty, and it
was not overriden. Let's change that.

Signed-off-by: Giulia Naponiello gnaponie@redhat.com

Looking at the code in find_parent_images_with_package, this may need to be more complex, no?

Can we just abstract that snippet into its own method and use it in both places?

def find_parent_image_from_child(self, child_image, children):                                                                                                                                                     
    parent_brew_build = child_image.get("parent_brew_build")                                    
    if parent_brew_build:                                                                       
        return parent_brew_build                                                                
    # We need to resolve the image in here because "parent_image_builds" needs to be there      
    # and it gets populated when the image gets resolved.                                       
    child_image.resolve(self, children)                                                         
    # If the parent is not in `parent_brew_build` we can try to look for the parent in Brew,    
    # using the field `parent_image_builds` (searching for the nvr), which should always be there.
    # In case parent_brew_build is None and child_image["parent_image_builds"] == {},           
    # it means we found a base image and there's no parent image.                               
    if not parent_brew_build and child_image["parent_image_builds"]:                            
        parent_brew_build = [                                                                   
            i["nvr"] for i in child_image["parent_image_builds"].values()                       
            if i["id"] == child_image["parent_build_id"]][0]                                    

    return parent_brew_build

Looking at the code in find_parent_images_with_package, this may need to be more complex, no?
Can we just abstract that snippet into its own method and use it in both places?
def find_parent_image_from_child(self, child_image, children):
parent_brew_build = child_image.get("parent_brew_build")
if parent_brew_build:
return parent_brew_build
# We need to resolve the image in here because "parent_image_builds" needs to be there
# and it gets populated when the image gets resolved.
child_image.resolve(self, children)
# If the parent is not in parent_brew_build we can try to look for the parent in Brew,
# using the field parent_image_builds (searching for the nvr), which should always be there.
# In case parent_brew_build is None and child_image["parent_image_builds"] == {},
# it means we found a base image and there's no parent image.
if not parent_brew_build and child_image["parent_image_builds"]:
parent_brew_build = [
i["nvr"] for i in child_image["parent_image_builds"].values()
if i["id"] == child_image["parent_build_id"]][0] 

return parent_brew_build

And if you can't find a parent image, I wouldn't raise an error. Otherwise, it may happen for base images or images from scratch. It's probably safe to assume that if you can't find a parent image either in lightblue or based on Koji's build metadata, then there isn't one.

Sounds good, but why do you want to return return parent_brew_build right away if you find it? We should resolve the image first.

Sounds good, but why do you want to return return parent_brew_build right away if you find it? We should resolve the image first.

Oh! I didn't realize resolving was always needed. If it is, then just remove that return :)

Mmm but still we need to resolve the parent image, and not the child image.
But I get your point, we should not repeat the code. I can come up with the change and push it.

Oh Lui, I think I realized only now what you meant. I'll address the comments now.

rebased onto a94530711a59c581f236bad95479d0cfd4375e76
4 years ago

I think we should remove this else-block entirely.

+ # it means we found a base image and there's no parent image.
+ if not parent_brew_build and child_image["parent_image_builds"]:

not parent_brew_build can be removed from this if.
Edited 4 years ago by cqi

+ child_image.resolve(self, children)

For find_parent_image_from_child, only need to resolve the image to get parent_image_builds from corresponding Brew build. ContainerImage.resolve resolves three kind of things and parent_image_builds is resolved in resolve_commit. Can we just make a call child_image.resolve_commit() here in order to save time from the other two "resolve" methods?
Edited 4 years ago by cqi

Suggest to write docstring for this method to describe the behaivor, input parameters and return value.
  • child_image.resolve(self, children)

For find_parent_image_from_child, only need to resolve the image to get parent_image_builds from corresponding Brew build. ContainerImage.resolve resolves three kind of things and parent_image_builds is resolved in resolve_commit. Can we just make a call child_image.resolve_commit() here in order to save time from the other two "resolve" methods?

Good catch. We can get rid of the "children" parameter this way.

rebased onto f1c4eaa3b716fd1e438ca7650ecac1ddda49d3fa
4 years ago

rebased onto 0c5813676c0b346700069db37e77646dfec026e1
4 years ago

rebased onto 34108db
4 years ago

:thumbsup: LGTM!

Commit f860bfd fixes this pull-request

Pull-Request has been merged by gnaponie
4 years ago

Pull-Request has been merged by gnaponie
4 years ago
Changes Summary 2
+36 -12
file changed
freshmaker/lightblue.py
+36 -0
file changed
tests/test_lightblue.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.