#2359 hub: importImage doesn't honor volume
Merged 2 years ago by mikem. Opened 2 years ago by tkopecek.
tkopecek/koji issue2355  into  master

file modified
+5 -6
@@ -9874,7 +9874,7 @@ 

                  'rpmdiff output was:\n%s' % (os.path.basename(first_rpm), d.textdiff()))

  

  

- def importImageInternal(task_id, build_id, imgdata):

+ def importImageInternal(task_id, build_info, imgdata):

      """

      Import image info and the listing into the database, and move an image

      to the final resting place. The filesize may be reported as a string if it
@@ -9897,7 +9897,6 @@ 

      koji.plugin.run_callbacks('preImport', type='image', image=imgdata)

  

      # import the build output

-     build_info = get_build(build_id, strict=True)

      workpath = koji.pathinfo.task(imgdata['task_id'])

      imgdata['relpath'] = koji.pathinfo.taskrelpath(imgdata['task_id'])

      archives = []
@@ -13775,7 +13774,7 @@ 

              build_info['volume_name'] = vol['name']

              vol_update = True

  

-         self.importImage(task_id, build_id, results)

+         self.importImage(task_id, build_info, results)

          ensure_volume_symlink(build_info)

  

          st_old = build_info['state']
@@ -14160,7 +14159,7 @@ 

              _untag_build(fromtag, build, user_id=user_id, force=force, strict=True)

          _tag_build(tag, build, user_id=user_id, force=force)

  

-     def importImage(self, task_id, build_id, results):

+     def importImage(self, task_id, build_info, results):

          """

          Import a built image, populating the database with metadata and

          moving the image to its final location.
@@ -14169,11 +14168,11 @@ 

              if 'task_id' not in sub_results:

                  logger.warning('Task %s failed, no image available' % task_id)

                  continue

-             importImageInternal(task_id, build_id, sub_results)

+             importImageInternal(task_id, build_info, sub_results)

              if 'rpmresults' in sub_results:

                  rpm_results = sub_results['rpmresults']

                  _import_wrapper(rpm_results['task_id'],

-                                 get_build(build_id, strict=True), rpm_results)

+                                 get_build(build_info['id'], strict=True), rpm_results)

  

      def tagNotification(self, is_successful, tag_id, from_id, build_id, user_id,

                          ignore_success=False, failure_msg=''):

@@ -38,11 +38,14 @@ 

          context.session.host_id = 42

          get_build.return_value = {

              'id': 2,

+             'name': 'name',

+             'version': 'version',

+             'release': 'release',

          }

          get_archive_type.return_value = 4

          work.return_value = self.tempdir

          os.makedirs(self.tempdir + "/tasks/1/1")

-         kojihub.importImageInternal(task_id=1, build_id=2, imgdata=imgdata)

+         kojihub.importImageInternal(task_id=1, build_info=get_build.return_value, imgdata=imgdata)

  

      @mock.patch('kojihub.get_rpm')

      @mock.patch('koji.pathinfo.build')
@@ -76,10 +79,16 @@ 

              ],

              'rpmlist': [rpm],

          }

+         build_info = {

+             'name': 'name',

+             'version': 'version',

+             'release': 'release',

+             'id': 2

+         }

          cursor = mock.MagicMock()

          context.cnx.cursor.return_value = cursor

          context.session.host_id = 42

-         get_build.return_value = {'id': 2}

+         get_build.return_value = build_info

          get_rpm.return_value = rpm

          get_archive_type.return_value = 4

          work.return_value = self.tempdir
@@ -94,7 +103,7 @@ 

          with open(workdir + '/foo.log', 'w'):

              pass

  

-         kojihub.importImageInternal(task_id=1, build_id=2, imgdata=imgdata)

+         kojihub.importImageInternal(task_id=1, build_info=build_info, imgdata=imgdata)

  

          # Check that the log symlink made it to where it was supposed to.

          dest = os.readlink(workdir + '/foo.log')

We're not sending updated build_info to the method and it is requesting
data from db. There is not volume saved yet.

Change is reusing modified buildinfo.

Fixes: https://pagure.io/koji/issue/2355

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

1 new commit added

  • fix tests
2 years ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

2 years ago

It's always a red flag for me when I see an established function change signature. This signature change is more than cosmetic as the underlying behavior around the rename argument changes too (necessarily).

However, I think this is OK, but let me explain my thinking while I'm here.

  • importImageInternal is a purely internal function on the hub
  • it is only called from one place (Host.importImage), which is in turn only called from one place (Host.completeImageBuild).

Given that the only place we're called from is passing in a deliberately modified build_info, it makes sense to rely on that instead of querying.

Commit 7d998fe fixes this pull-request

Pull-Request has been merged by mikem

2 years ago