#510 Convert frequent used image properties to real ContainerImage properties
Closed 4 years ago by mprahl. Opened 4 years ago by cqi.
cqi/freshmaker new-image-properties  into  master

@@ -23,7 +23,6 @@ 

  # Written by Jan Kaluza <jkaluza@redhat.com>

  

  import json

- import koji

  import kobo

  

  from freshmaker import conf, db
@@ -238,8 +237,8 @@ 

                      continue

  

                  self.log_debug("Recording %s", nvr)

-                 parent_nvr = image["parent"].nvr \

-                     if "parent" in image and image["parent"] else None

+ 

+                 parent_nvr = image.parent.nvr if image.parent else None

                  dep_on = builds[parent_nvr] if parent_nvr in builds else None

  

                  if parent_nvr:
@@ -261,8 +260,6 @@ 

                      state_reason = ""

                      state = ArtifactBuildState.PLANNED.value

  

-                 image_name = koji.parse_NVR(image.nvr)["name"]

- 

                  # Only released images are considered as directly affected for

                  # rebuild. If some image is not in the latest released version and

                  # it is included in a rebuild, it must be just a dependency of
@@ -273,7 +270,7 @@ 

                      rebuild_reason = RebuildReason.DEPENDENCY.value

  

                  build = self.record_build(

-                     event, image_name, ArtifactType.IMAGE,

+                     event, image.name, ArtifactType.IMAGE,

                      dep_on=dep_on,

                      state=ArtifactBuildState.PLANNED.value,

                      original_nvr=nvr,
@@ -354,13 +351,10 @@ 

          :rtype: bool

          :return: True when image should be filtered out.

          """

- 

-         parsed_nvr = koji.parse_NVR(image.nvr)

- 

          if not self.event.is_allowed(

-                 self, image_name=parsed_nvr["name"],

-                 image_version=parsed_nvr["version"],

-                 image_release=parsed_nvr["release"]):

+                 self, image_name=image.name,

+                 image_version=image.version,

+                 image_release=image.release):

              self.log_info(

                  "Skipping rebuild of image %s, not allowed by configuration",

                  image.nvr)

file modified
+46 -28
@@ -132,25 +132,47 @@ 

  

      region = dogpile.cache.make_region().configure(conf.dogpile_cache_backend)

  

-     @classmethod

-     def create(cls, data):

-         image = cls()

-         image.update(data)

+     def __init__(self, *args, **kwargs):

+         super().__init__(*args, **kwargs)
qwan commented 4 years ago

Can you update this? because dict expect at most one argument, and it can accept either one argument or name=value pairs, but not both.

cqi commented 4 years ago

Can you update this? because dict expect at most one argument, and it can accept either one argument or name=value pairs, but not both.

This __init__ inherits the ability of dict to initialize an object in these two ways:

image = ContainerImage({...})
image = ContainerImage(name=.., version=.., ...)

To keep the usage flexible, I'd like to keep this basic ability for ContainerImage as well.

+         self.update(*args, **kwargs)

  

-         arch = data.get('architecture')

-         image['multi_arch_rpm_manifest'] = {}

-         rpm_manifest = data.get('rpm_manifest')

+         self.nvr = self.get('brew', {}).get('build')

+         self._parsed_nvr = kobo.rpmlib.parse_nvr(self.nvr) if self.nvr else None

+ 

+         arch = self.get('architecture')

+         rpm_manifest = self.get('rpm_manifest')

+         self['multi_arch_rpm_manifest'] = {}

          if arch and rpm_manifest:

-             image['multi_arch_rpm_manifest'][arch] = rpm_manifest

+             self['multi_arch_rpm_manifest'][arch] = rpm_manifest

  

-         return image

+     @property

+     def name(self):

+         if not self._parsed_nvr:

+             raise ValueError('No brew/build is set for this image.')

minor: I'd remove the '/' between 'brew/build'

+         return self._parsed_nvr['name']

  

-     def __hash__(self):

-         return hash((self.nvr))

+     @property

+     def version(self):

+         if not self._parsed_nvr:

+             raise ValueError('No brew/build is set for this image.')

+         return self._parsed_nvr['version']

  

      @property

-     def nvr(self):

-         return self['brew']['build']

+     def release(self):

+         if not self._parsed_nvr:

+             raise ValueError('No brew/build is set for this image.')

+         return self._parsed_nvr['release']

+ 

+     @property

+     def parent(self):

+         return self.get('parent')

+ 

+     @parent.setter

+     def parent(self, value):

+         self['parent'] = value

+ 

+     def __hash__(self):

+         return hash((self.nvr))

  

      def log_error(self, err):

          """
@@ -210,7 +232,7 @@ 

      def _get_additional_data_from_koji(self, nvr):

          """

          Finds the build defined by `nvr` in Koji and returns dict with

-         additional information about this build including "repository",

+          information about this build including "repository",

          "commit", "target" and "git_branch".

  

          In case of lookup error, the "error" will be set to error string.
@@ -581,7 +603,7 @@ 

          images = []

          nvr_to_arches = {}

          for image_data in response['processed']:

-             image = ContainerImage.create(image_data)

+             image = ContainerImage(image_data)

              images.append(image)

  

              # TODO: In the future, we may want to combine different ContainerImage
@@ -1123,7 +1145,7 @@ 

  

          if images:

              if parent_image:

-                 images[-1]['parent'] = parent_image

+                 images[-1].parent = parent_image

              else:

                  # If we did not find the parent image with the package,

                  # we still want to set the parent of the last image with
@@ -1140,7 +1162,7 @@ 

                      log.error(err)

                      if not images[-1]['error']:

                          images[-1]['error'] = err

-                 images[-1]['parent'] = parent

+                 images[-1].parent = parent

  

          if not parent_image:

              return images
@@ -1368,19 +1390,16 @@ 

                  if phase == "handle_parent_change":

                      # Find out the name of parent image of latest release image.

                      latest_image = nvr_to_image[latest_released_nvr]

-                     if "parent" not in latest_image or not latest_image["parent"]:

+                     if not latest_image.parent:

                          continue

-                     latest_parent_name = koji.parse_NVR(

-                         latest_image["parent"].nvr)["name"]

  

                      # Go through the older images and in case the parent image differs,

                      # update its parents according to latest image parents.

                      for nvr in nvrs[latest_released_nvr_index + 1:]:

                          image = nvr_to_image[nvr]

-                         if "parent" not in image or not image["parent"]:

+                         if not image.parent:

                              continue

-                         parent_name = koji.parse_NVR(image["parent"].nvr)["name"]

-                         if parent_name != latest_parent_name:

+                         if image.parent.name != latest_image.parent.name:

                              for image_id, parent_id in nvr_to_coordinates[nvr]:

                                  latest_image_id, latest_parent_id = nvr_to_coordinates[latest_released_nvr][0]

                                  to_rebuild[image_id][parent_id:] = to_rebuild[latest_image_id][latest_parent_id:]
@@ -1395,7 +1414,7 @@ 

                              # the ["parent"] record for the child image to point to the image

                              # with highest NVR.

                              if parent_id != 0:

-                                 to_rebuild[image_id][parent_id - 1]["parent"] = nvr_to_image[latest_released_nvr]

+                                 to_rebuild[image_id][parent_id - 1].parent = nvr_to_image[latest_released_nvr]

  

          return to_rebuild

  
@@ -1407,8 +1426,7 @@ 

          # included in different container repositories.

          repositories = image.get_registry_repositories(self)

          repository_key = sorted([r["repository"] for r in repositories])

-         parsed_nvr = koji.parse_NVR(image.nvr)

-         return "%s-%s-%s" % (parsed_nvr["name"], parsed_nvr["version"], repository_key)

+         return "%s-%s-%s" % (image.name, image.version, repository_key)

  

      def _images_to_rebuild_to_batches(self, to_rebuild, directly_affected_nvrs):

          """
@@ -1515,7 +1533,7 @@ 

                  rebuild_list[srpm_name] = self.find_parent_images_with_package(

                      image, srpm_name, [])

                  if rebuild_list[srpm_name]:

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

+                     image.parent = rebuild_list[srpm_name][0]

                  else:

                      parent_brew_build = self.find_parent_brew_build_nvr_from_child(image)

                      if parent_brew_build:
@@ -1523,7 +1541,7 @@ 

                          if parent:

                              parent = parent[0]

                              parent.resolve(self, images)

-                             image['parent'] = parent

+                             image.parent = parent

                  rebuild_list[srpm_name].insert(0, image)

              return rebuild_list

  

@@ -911,8 +911,8 @@ 

              self.assertEqual(build.type, ArtifactType.IMAGE.value)

  

              image = images[build.original_nvr]

-             if image['parent']:

-                 self.assertEqual(build.dep_on.original_nvr, image['parent']['brew']['build'])

+             if image.parent:

+                 self.assertEqual(build.dep_on.original_nvr, image.parent.nvr)

              else:

                  self.assertEqual(build.dep_on, None)

  

file modified
+50 -50
@@ -153,7 +153,7 @@ 

          self.patcher = helpers.Patcher(

              'freshmaker.lightblue.')

  

-         self.dummy_image = ContainerImage.create({

+         self.dummy_image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'completion_date': u'20170421T04:27:51.000-0400',
@@ -180,7 +180,7 @@ 

          self.koji_read_config_patcher.stop()

  

      def test_create(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'completion_date': '20151210T10:09:35.000-0500',
@@ -194,7 +194,7 @@ 

  

      def test_update_multi_arch(self):

          rpm_manifest_x86_64 = [{'rpms': [{'name': 'spam'}]}]

-         image_x86_64 = ContainerImage.create({

+         image_x86_64 = ContainerImage({

              '_id': '1233829',

              'architecture': 'amd64',

              'brew': {
@@ -206,7 +206,7 @@ 

          })

  

          rpm_manifest_s390x = [{'rpms': [{'name': 'maps'}]}]

-         image_s390x = ContainerImage.create({

+         image_s390x = ContainerImage({

              '_id': '1233829',

              'architecture': 's390x',

              'brew': {
@@ -244,7 +244,7 @@ 

          })

  

      def test_log_error(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              'brew': {

                  'build': 'package-name-1-4-12.10',

              },
@@ -366,7 +366,7 @@ 

              "Cannot resolve the container image: Expected exception.")

  

      def test_resolve_content_sets_already_included_in_lb_response(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -384,7 +384,7 @@ 

                           "lightblue_container_image")

  

      def test_resolve_content_sets_no_repositories(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -403,7 +403,7 @@ 

      @patch('freshmaker.kojiservice.KojiService.get_task_request')

      def test_resolve_content_sets_no_repositories_children_set(

              self, get_task_request, get_build):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -414,14 +414,14 @@ 

          })

          self.assertTrue("content_sets" not in image)

  

-         child1 = ContainerImage.create({

+         child1 = ContainerImage({

              '_id': '1233828',

              'brew': {

                  'build': 'child1-name-1-4-12.10',

              },

          })

  

-         child2 = ContainerImage.create({

+         child2 = ContainerImage({

              '_id': '1233828',

              'brew': {

                  'build': 'child2-name-1-4-12.10',
@@ -434,7 +434,7 @@ 

          self.assertEqual(image["content_sets"], ["foo", "bar"])

  

      def test_resolve_content_sets_empty_repositories(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -451,7 +451,7 @@ 

          self.assertEqual(image["content_sets"], [])

  

      def test_resolve_published(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -467,7 +467,7 @@ 

              include_rpm_manifest=False)

  

      def test_resolve_published_unpublished(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -485,7 +485,7 @@ 

          self.assertEqual(image["rpm_manifest"], "x")

  

      def test_resolve_published_not_image_in_lb(self):

-         image = ContainerImage.create({

+         image = ContainerImage({

              '_id': '1233829',

              'brew': {

                  'build': 'package-name-1-4-12.10',
@@ -735,19 +735,19 @@ 

          ]

  

          self.fake_container_images = [

-             ContainerImage.create(data)

+             ContainerImage(data)

              for data in self.fake_images_with_parsed_data]

  

          self.fake_container_images_floating_tag = [

-             ContainerImage.create(data)

+             ContainerImage(data)

              for data in self.fake_images_with_parsed_data_floating_tag]

  

          self.fake_images_with_modules = [

-             ContainerImage.create(data)

+             ContainerImage(data)

              for data in self.fake_images_with_modules]

  

          self.fake_container_images_with_parent_brew_build = [

-             ContainerImage.create(data)

+             ContainerImage(data)

              for data in self.fake_images_with_parent_brew_build]

  

          self.fake_koji_builds = [{"task_id": 654321}, {"task_id": 123456}]
@@ -1230,7 +1230,7 @@ 

          cont_repos.return_value = self.fake_repositories_with_content_sets

          # "filtered_x-1-23" image will be filtered by filter_fnc.

          cont_images.return_value = self.fake_container_images + [

-             ContainerImage.create(

+             ContainerImage(

                  {"content_sets": ["dummy-content-set-1"],

                   "brew": {"build": "filtered_x-1-23"},

                   "repositories": [
@@ -1323,7 +1323,7 @@ 

  

          # "filtered_x-1-23" image will be filtered by filter_fnc.

          cont_images.return_value = self.fake_container_images + [

-             ContainerImage.create(

+             ContainerImage(

                  {

                      "content_sets": ["dummy-content-set-1"],

                      "brew": {"build": "filtered_x-1-23"},
@@ -1460,7 +1460,7 @@ 

          # set, it will take the content_sets from the child image.

          images_without_repositories = []

          for data in self.fake_images_with_parsed_data:

-             img = ContainerImage.create(data)

+             img = ContainerImage(data)

              del img["repositories"]

              images_without_repositories.append(img)

  
@@ -1495,7 +1495,7 @@ 

          # set, it will take the content_sets from the child image.

          images_without_repositories = []

          for data in self.fake_images_with_parsed_data:

-             img = ContainerImage.create(data)

+             img = ContainerImage(data)

              del img["repositories"]

              images_without_repositories.append(img)

  
@@ -1528,62 +1528,62 @@ 

                                 find_images_with_packages_from_content_set):

          exists.return_value = True

  

-         image_a = ContainerImage.create({

+         image_a = ContainerImage({

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

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

              'repository': 'repo-1',

              'commit': 'image-a-commit'

          })

-         image_b = ContainerImage.create({

+         image_b = ContainerImage({

              'brew': {'package': 'image-b', 'build': 'image-b-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-b-commit',

              'parent': image_a,

          })

-         image_c = ContainerImage.create({

+         image_c = ContainerImage({

              'brew': {'package': 'image-c', 'build': 'image-c-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-c-commit',

              'parent': image_b,

          })

-         image_e = ContainerImage.create({

+         image_e = ContainerImage({

              'brew': {'package': 'image-e', 'build': 'image-e-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-e-commit',

              'parent': image_a,

          })

-         image_d = ContainerImage.create({

+         image_d = ContainerImage({

              'brew': {'package': 'image-d', 'build': 'image-d-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-d-commit',

              'parent': image_e,

          })

-         image_j = ContainerImage.create({

+         image_j = ContainerImage({

              'brew': {'package': 'image-j', 'build': 'image-j-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-j-commit',

              'parent': image_e,

          })

-         image_k = ContainerImage.create({

+         image_k = ContainerImage({

              'brew': {'package': 'image-k', 'build': 'image-k-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-k-commit',

              'parent': image_j,

          })

-         image_g = ContainerImage.create({

+         image_g = ContainerImage({

              'brew': {'package': 'image-g', 'build': 'image-g-v-r1'},

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

              'repository': 'repo-1',

              'commit': 'image-g-commit',

              'parent': None,

          })

-         image_f = ContainerImage.create({

+         image_f = ContainerImage({

              'brew': {'package': 'image-f', 'build': 'image-f-v-r1'},

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

              'repository': 'repo-1',
@@ -1591,49 +1591,49 @@ 

              'parent': image_g,

          })

  

-         leaf_image1 = ContainerImage.create({

+         leaf_image1 = ContainerImage({

              'brew': {'build': 'leaf-image-1-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image1-commit',

          })

-         leaf_image2 = ContainerImage.create({

+         leaf_image2 = ContainerImage({

              'brew': {'build': 'leaf-image-2-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image2-commit',

          })

-         leaf_image3 = ContainerImage.create({

+         leaf_image3 = ContainerImage({

              'brew': {'build': 'leaf-image-3-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image3-commit',

          })

-         leaf_image4 = ContainerImage.create({

+         leaf_image4 = ContainerImage({

              'brew': {'build': 'leaf-image-4-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image4-commit',

          })

-         leaf_image5 = ContainerImage.create({

+         leaf_image5 = ContainerImage({

              'brew': {'build': 'leaf-image-5-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image5-commit',

          })

-         leaf_image6 = ContainerImage.create({

+         leaf_image6 = ContainerImage({

              'brew': {'build': 'leaf-image-6-1'},

              'parsed_data': {'layers': ['fake layer']},

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

              'repository': 'repo-1',

              'commit': 'leaf-image6-commit',

          })

-         leaf_image7 = ContainerImage.create({

+         leaf_image7 = ContainerImage({

              'brew': {'build': 'leaf-image-7-1'},

              'parsed_data': {'layers': ['fake layer']},

              'repositories': [{'repository': 'foo/bar'}],
@@ -1656,7 +1656,7 @@ 

          find_images_with_packages_from_content_set.return_value = images

  

          leaf_image6_as_parent = copy.deepcopy(leaf_image6)

-         leaf_image6_as_parent['parent'] = image_f

+         leaf_image6_as_parent.parent = image_f

          # When the image is a parent, directly_affected is not set

          del leaf_image6_as_parent["directly_affected"]

          find_parent_images_with_package.side_effect = [
@@ -1739,7 +1739,7 @@ 

              find_images_with_packages_from_content_set, cont_images):

          exists.return_value = True

  

-         image_a = ContainerImage.create({

+         image_a = ContainerImage({

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

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

              "repository": "repo-1",
@@ -1789,16 +1789,16 @@ 

              }]

          }

  

-         directly_affected_ubi_image = ContainerImage.create(copy.deepcopy(ubi_image_template))

+         directly_affected_ubi_image = ContainerImage(copy.deepcopy(ubi_image_template))

  

          dependency_ubi_image_data = copy.deepcopy(ubi_image_template)

          dependency_ubi_image_nvr = directly_affected_ubi_image.nvr + ".12345678"

          dependency_ubi_image_data["brew"]["build"] = dependency_ubi_image_nvr

          # A dependecy image is not directly published

          dependency_ubi_image_data["repositories"] = []

-         dependency_ubi_image = ContainerImage.create(dependency_ubi_image_data)

+         dependency_ubi_image = ContainerImage(dependency_ubi_image_data)

  

-         python_image = ContainerImage.create({

+         python_image = ContainerImage({

              "brew": {"package": "python-36-container", "build": "python-36-container-1-10"},

              "parent_brew_build": directly_affected_ubi_image.nvr,

              "parent_image_builds": {},
@@ -1812,7 +1812,7 @@ 

              }]

          })

  

-         nodejs_image = ContainerImage.create({

+         nodejs_image = ContainerImage({

              "brew": {"package": "nodejs-12-container", "build": "nodejs-12-container-1-20.45678"},

              "parent_brew_build": dependency_ubi_image.nvr,

              "repository": "containers/nodejs-12",
@@ -2029,7 +2029,7 @@ 

                  'architecture': 's390x'

              }

          ]

-         new_images = [ContainerImage.create(i) for i in new_images]

+         new_images = [ContainerImage(i) for i in new_images]

          find_repos.return_value = self.fake_repositories_with_content_sets

          find_images.return_value = self.fake_container_images + new_images

          right_content_sets = [["dummy-content-set-1"],
@@ -2088,13 +2088,13 @@ 

          repositories = {

              repo["repository"]: repo for repo in

              self.fake_repositories_with_content_sets}

-         parent = ContainerImage.create({

+         parent = ContainerImage({

              "brew": {"build": "parent-1-2"}, "repositories": repos,

              "content_sets": ["dummy-content-set-1"]})

-         latest_parent = ContainerImage.create({

+         latest_parent = ContainerImage({

              "brew": {"build": "parent-1-3"}, "repositories": repos,

              "content_sets": ["dummy-content-set-1"]})

-         older_parent = ContainerImage.create({

+         older_parent = ContainerImage({

              "brew": {"build": "parent-1-1"}, "repositories": repos,

              "content_sets": ["dummy-content-set-2"]})

          cont_images.return_value = [parent, latest_parent, older_parent]
@@ -2181,7 +2181,7 @@ 

          self.os_path_exists_patcher.stop()

  

      def _create_img(self, nvr):

-         return ContainerImage.create({

+         return ContainerImage({

              'brew': {'build': nvr},

              'content_sets': [],

              'content_sets_source': 'distgit',
@@ -2198,7 +2198,7 @@ 

              else:

                  image = self._create_img(data)

              if images:

-                 images[len(images) - 1]['parent'] = image

+                 images[len(images) - 1].parent = image

              images.append(image)

          return images

  

file modified
+1 -1
@@ -270,7 +270,7 @@ 

              u'summary': u'Java logging toolkit',

              u'version': u'2.1'

          }]}]

-         return ContainerImage.create({

+         return ContainerImage({

              u'arches': arches,  # Populated based on Brew build

              u'architecture': architecture,  # Populated from Lightblue data

              u'rpm_manifest': rpm_manifest,

An image's name, version and parent are accessed frequently by parsing
from ContainerImage.nvr and key parent. This change proposed a new way
to access these properties by making properties of ContainerImage, e.g.
ContainerImage.name and ContainerImage.parent. With these changes, the
original code becomes shorter and less calls to the function to parse
N-V-R from an image's brew build NVR.

This proposed change removes an interface ContainerImage.create that I
wrote before. After reviewing this piece of code again, I feel it's
useless. Instead, a normal construction just works well. For example to
initiate an image object, the code could be:

image = ContainerImage({'brew': {'build': 'n-v-r'}})

Signed-off-by: Chenxiong Qi cqi@redhat.com

rebased onto 62a44dfaf32ab13cc35e51a229b7ec06471dde91

4 years ago

@cqi there are conflicts, can you fix them?

rebased onto 11fa4e6

4 years ago

@gnaponie Rebased and fixed the conflicts. PTAL.

dict doesn't accept non-keyworded arguments.

@cqi could you please solve the commits also here? Then we can merge this. Thank you

rebased onto 391d8e2

4 years ago

rebased onto 7f7737d

4 years ago

Sorry for late response. Patch is rebased and conflicts are fixed.

@qwan @jkaluza

The *args is for the case ContainerImage({...}).

rebased onto 5684157

4 years ago

Can you update this? because dict expect at most one argument, and it can accept either one argument or name=value pairs, but not both.

One minor comment, other looks good to me.

Can you update this? because dict expect at most one argument, and it can accept either one argument or name=value pairs, but not both.

This __init__ inherits the ability of dict to initialize an object in these two ways:

image = ContainerImage({...})
image = ContainerImage(name=.., version=.., ...)

To keep the usage flexible, I'd like to keep this basic ability for ContainerImage as well.

To keep the usage flexible, I'd like to keep this basic ability for ContainerImage as well.

Since both dict.__init__ and dict.update don't accept multiple arguments, it doesn't make sense to have such code. I'd suggest something like:

    def __init__(self, data=None, **kwargs):
        super().__init__(**kwargs)
        if data:
            self.update(data)

minor: I'd remove the '/' between 'brew/build'

Just a minor comment, but it looks good to me +1

Freshmaker has been migrated to GitHub. Please reopen your PR on GitHub.

Pull-Request has been closed by mprahl

4 years ago