#1094 Use anonymous Koji session properly
Merged 6 months ago by mprahl. Opened 6 months ago by cqi.
cqi/fm-orchestrator use-anonymous-koji-session  into  master

@@ -48,9 +48,9 @@ 

  logging.basicConfig(level=logging.DEBUG)

  

  

- def get_session(config, owner):

+ def get_session(config, owner, login=True):

      from module_build_service.builder.KojiModuleBuilder import KojiModuleBuilder

-     return KojiModuleBuilder.get_session(config, owner)

+     return KojiModuleBuilder.get_session(config, owner, login=login)

  

  

  def strip_suffixes(s, suffixes):

@@ -228,7 +228,7 @@ 

      def _koji_rpms_in_tag(self, tag):

          """ Return the list of koji rpms in a tag. """

          log.debug("Listing rpms in koji tag %s", tag)

-         session = get_session(self.config, self.owner)

+         session = get_session(self.config, self.owner, login=False)

  

          try:

              rpms, builds = session.listTaggedRPMS(tag, latest=True)

@@ -307,7 +307,7 @@ 

                  }

              }

          }

-         session = get_session(self.config, None)

+         session = get_session(self.config, None, login=False)

          # Only add the CG build owner if the user exists in Koji

          if session.getUser(self.owner):

              ret[u'owner'] = self.owner

@@ -237,7 +237,7 @@ 

              reusable_module = get_reusable_module(db_session, module_build)

              if not reusable_module:

                  return filtered_rpms

-             koji_session = KojiModuleBuilder.get_session(conf, None)

+             koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

              # Get all the RPMs and builds of the reusable module in Koji

              rpms, builds = koji_session.listTaggedRPMS(reusable_module.koji_tag, latest=True)

              # Convert the list to a dict where each key is the build_id

@@ -445,7 +445,7 @@ 

  

      @staticmethod

      @module_build_service.utils.retry(wait_on=(xmlrpclib.ProtocolError, koji.GenericError))

-     def get_session(config, owner):

+     def get_session(config, owner, login=True):

It seems like owner is no longer used. Do you mind removing this parameter since you're modifying this anyways? If you don't have time, we can make this a separate PR.

          koji_config = munch.Munch(koji.read_config(

              profile_name=config.koji_profile,

              user_config=config.koji_config,

@@ -454,31 +454,34 @@ 

          koji_config["timeout"] = 60 * 10

  

          address = koji_config.server

-         authtype = koji_config.authtype

-         log.info("Connecting to koji %r with %r." % (address, authtype))

+         log.info("Connecting to koji %r.", address)

          koji_session = koji.ClientSession(address, opts=koji_config)

-         if authtype == "kerberos":

-             ccache = getattr(config, "krb_ccache", None)

-             keytab = getattr(config, "krb_keytab", None)

-             principal = getattr(config, "krb_principal", None)

-             log.debug("  ccache: %r, keytab: %r, principal: %r" % (

-                 ccache, keytab, principal))

-             if keytab and principal:

-                 koji_session.krb_login(

-                     principal=principal,

-                     keytab=keytab,

-                     ccache=ccache

+ 

+         if login:

You could avoid this indentation by just doing:

if not login:
    return koji_session

authtype = koji_config.authtype
...

+             authtype = koji_config.authtype

+             log.info("Authenticate session with %r.", authtype)

+             if authtype == "kerberos":

+                 ccache = getattr(config, "krb_ccache", None)

+                 keytab = getattr(config, "krb_keytab", None)

+                 principal = getattr(config, "krb_principal", None)

+                 log.debug("  ccache: %r, keytab: %r, principal: %r" % (

+                     ccache, keytab, principal))

+                 if keytab and principal:

+                     koji_session.krb_login(

+                         principal=principal,

+                         keytab=keytab,

+                         ccache=ccache

+                     )

+                 else:

+                     koji_session.krb_login(ccache=ccache)

+             elif authtype == "ssl":

+                 koji_session.ssl_login(

+                     os.path.expanduser(koji_config.cert),

+                     None,

+                     os.path.expanduser(koji_config.serverca)

                  )

              else:

-                 koji_session.krb_login(ccache=ccache)

-         elif authtype == "ssl":

-             koji_session.ssl_login(

-                 os.path.expanduser(koji_config.cert),

-                 None,

-                 os.path.expanduser(koji_config.serverca)

-             )

-         else:

-             raise ValueError("Unrecognized koji authtype %r" % authtype)

+                 raise ValueError("Unrecognized koji authtype %r" % authtype)

  

          return koji_session

  

@@ -1084,7 +1087,7 @@ 

          """

          # If the component has not been built before, then None is returned. Instead, let's

          # return 0.0 so the type is consistent

-         koji_session = KojiModuleBuilder.get_session(conf, None)

+         koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

          return koji_session.getAverageBuildDuration(component) or 0.0

  

      @classmethod

@@ -1191,7 +1194,7 @@ 

              build = models.ModuleBuild.get_build_from_nsvc(

                  db_session, mmd.get_name(), mmd.get_stream(), mmd.get_version(),

                  mmd.get_context())

-             koji_session = KojiModuleBuilder.get_session(conf, None)

+             koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

              rpms = koji_session.listTaggedRPMS(build.koji_tag, latest=True)[0]

              nvrs = set(kobo.rpmlib.make_nvr(rpm, force_epoch=True) for rpm in rpms)

              return list(nvrs)

@@ -1214,7 +1217,7 @@ 

          :return: koji tag

          """

  

-         session = KojiModuleBuilder.get_session(conf, None)

+         session = KojiModuleBuilder.get_session(conf, None, login=False)

          rpm_md = session.getRPM(rpm)

          if not rpm_md:

              return None

@@ -588,7 +588,8 @@ 

          if not self.koji_session:

              # If Koji is not configured on the system, then just return 0.0 for components

              try:

-                 self.koji_session = KojiModuleBuilder.get_session(self.config, self.owner)

+                 self.koji_session = KojiModuleBuilder.get_session(

+                     self.config, self.owner, login=False)

                  # If the component has not been built before, then None is returned. Instead,

                  # let's return 0.0 so the type is consistent

                  return self.koji_session.getAverageBuildDuration(component.package) or 0.0

@@ -35,6 +35,7 @@ 

  import module_build_service.scheduler.consumer

  from module_build_service import conf, models, log

  from module_build_service.builder import GenericBuilder

+ from module_build_service.builder.KojiModuleBuilder import KojiModuleBuilder

  

  

  class MBSProducer(PollingProducer):

@@ -66,8 +67,7 @@ 

  

          if conf.system == 'koji':

              # We don't do this on behalf of users

-             koji_session = module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder\

-                 .get_session(conf, None)

+             koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

              log.info('Querying tasks for statuses:')

              res = models.ComponentBuild.query.filter_by(

                  state=koji.BUILD_STATES['BUILDING']).options(

@@ -302,8 +302,7 @@ 

  

          now = datetime.utcnow()

  

-         koji_session = module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder\

-             .get_session(config, None)

+         koji_session = KojiModuleBuilder.get_session(config, None)

          for target in koji_session.getBuildTargets():

              koji_tag = target["dest_tag_name"]

              module = session.query(models.ModuleBuild).filter_by(

@@ -126,7 +126,7 @@ 

      :rtype: list[Modulemd.Module]

      """

      from module_build_service.builder.KojiModuleBuilder import KojiModuleBuilder

-     koji_session = KojiModuleBuilder.get_session(conf, None)

+     koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

      repos = koji_session.getExternalRepoList(tag)

      build_tags = find_build_tags_from_external_repos(koji_session, repos)

      if not build_tags:

@@ -265,7 +265,7 @@ 

      resolver = GenericResolver.create(conf)

  

      built_rpms = []

-     koji_session = KojiModuleBuilder.get_session(conf, None)

+     koji_session = KojiModuleBuilder.get_session(conf, None, login=False)

  

      for nsvc in modules_nsvc:

          name, stream, version, context = nsvc.split(':')

file modified
+57 -32

@@ -95,6 +95,16 @@ 

          self.config.koji_repository_url = conf.koji_repository_url

          self.module = module_build_service.models.ModuleBuild.query.filter_by(id=2).one()

  

+         self.p_read_config = patch('koji.read_config', return_value={

+             'authtype': 'kerberos',

+             'timeout': 60,

+             'server': 'http://koji.example.com/'

+         })

+         self.mock_read_config = self.p_read_config.start()

+ 

+     def teardown_method(self, test_method):

+         self.p_read_config.stop()

+ 

      def test_tag_to_repo(self):

          """ Test that when a repo msg hits us and we have no match,

          that we do nothing gracefully.

@@ -314,9 +324,9 @@ 

          expected_calls = [mock.call(1, 'foo'), mock.call(2, 'foo'), mock.call(1, 'bar')]

          assert mock_session.untagBuild.mock_calls == expected_calls

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights(self, get_session):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_build_weights(self, ClientSession):

+         session = ClientSession.return_value

          session.getLoggedInUser.return_value = {"id": 123}

          session.multiCall.side_effect = [

              # getPackageID response

@@ -327,7 +337,6 @@ 

              [[{'1': [], '2': [], '3': [{'weight': 1.0}, {'weight': 1.0}]}],

               [{'1': [], '2': [], '3': [{'weight': 1.0}, {'weight': 1.0}]}]]

          ]

-         get_session.return_value = session

  

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 2, "apr": 2}

@@ -335,9 +344,12 @@ 

          expected_calls = [mock.call(456), mock.call(789)]

          assert session.getTaskDescendents.mock_calls == expected_calls

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights_no_task_id(self, get_session):

-         session = MagicMock()

+         # getLoggedInUser requires to a logged-in session

+         session.krb_login.assert_called_once()

+ 

+     @patch('koji.ClientSession')

+     def test_get_build_weights_no_task_id(self, ClientSession):

+         session = ClientSession.return_value

          session.getLoggedInUser.return_value = {"id": 123}

          session.multiCall.side_effect = [

              # getPackageID response

@@ -348,17 +360,17 @@ 

              [[{'1': [], '2': [], '3': [{'weight': 1.0}, {'weight': 1.0}]}]]

          ]

          session.getAverageBuildDuration.return_value = None

-         get_session.return_value = session

  

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 2, "apr": 1.5}

  

          expected_calls = [mock.call(456)]

          assert session.getTaskDescendents.mock_calls == expected_calls

+         session.krb_login.assert_called_once()

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights_no_build(self, get_session):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_build_weights_no_build(self, ClientSession):

+         session = ClientSession.return_value

          session.getLoggedInUser.return_value = {"id": 123}

          session.multiCall.side_effect = [

              # getPackageID response

@@ -369,21 +381,20 @@ 

              [[{'1': [], '2': [], '3': [{'weight': 1.0}, {'weight': 1.0}]}]]

          ]

          session.getAverageBuildDuration.return_value = None

-         get_session.return_value = session

  

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 2, "apr": 1.5}

  

          expected_calls = [mock.call(456)]

          assert session.getTaskDescendents.mock_calls == expected_calls

+         session.krb_login.assert_called_once()

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights_listBuilds_failed(self, get_session):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_build_weights_listBuilds_failed(self, ClientSession):

+         session = ClientSession.return_value

          session.getLoggedInUser.return_value = {"id": 123}

          session.multiCall.side_effect = [[[1], [2]], []]

          session.getAverageBuildDuration.return_value = None

-         get_session.return_value = session

  

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 1.5, "apr": 1.5}

@@ -393,14 +404,14 @@ 

                            mock.call(packageID=2, userID=123, state=1,

                                      queryOpts={'limit': 1, 'order': '-build_id'})]

          assert session.listBuilds.mock_calls == expected_calls

+         session.krb_login.assert_called_once()

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights_getPackageID_failed(self, get_session):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_build_weights_getPackageID_failed(self, ClientSession):

+         session = ClientSession.return_value

          session.getLoggedInUser.return_value = {"id": 123}

          session.multiCall.side_effect = [[], []]

          session.getAverageBuildDuration.return_value = None

-         get_session.return_value = session

  

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 1.5, "apr": 1.5}

@@ -408,13 +419,15 @@ 

          expected_calls = [mock.call("httpd"), mock.call("apr")]

          assert session.getPackageID.mock_calls == expected_calls

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_build_weights_getLoggedInUser_failed(self, get_session):

-         session = MagicMock()

+         session.krb_login.assert_called_once()

+ 

+     @patch('koji.ClientSession')

+     def test_get_build_weights_getLoggedInUser_failed(self, ClientSession):

+         session = ClientSession.return_value

          session.getAverageBuildDuration.return_value = None

-         get_session.return_value = session

          weights = KojiModuleBuilder.get_build_weights(["httpd", "apr"])

          assert weights == {"httpd": 1.5, "apr": 1.5}

+         session.krb_login.assert_called_once()

  

      @patch.object(conf, 'base_module_arches',

                    new={"platform:xx": ["x86_64", "i686"]})

@@ -533,9 +546,9 @@ 

              expected_calls = []

          assert session.packageListBlock.mock_calls == expected_calls

  

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_built_rpms_in_module_build(self, get_session):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_built_rpms_in_module_build(self, ClientSession):

+         session = ClientSession.return_value

          session.listTaggedRPMS.return_value = ([

              {'build_id': 735939, 'name': 'tar', 'extra': None, 'arch': 'ppc64le',

               'buildtime': 1533299221, 'id': 6021394, 'epoch': 2, 'version': '1.30',

@@ -547,7 +560,6 @@ 

               'metadata_only': False, 'release': '4.el8+1308+551bfa71',

               'buildroot_id': 4321122, 'payloadhash': '0621ab2091256d21c47dcac868e7fc2a',

               'size': 878684}], [])

-         get_session.return_value = session

  

          # Module builds generated by init_data uses generic modulemd file and

          # the module's name/stream/version/context does not have to match it.

@@ -562,6 +574,7 @@ 

          ret = KojiModuleBuilder.get_built_rpms_in_module_build(mmd)

          assert set(ret) == set(

              ['bar-2:1.30-4.el8+1308+551bfa71', 'tar-2:1.30-4.el8+1308+551bfa71'])

+         session.assert_not_called()

  

      @pytest.mark.parametrize('br_filtered_rpms,expected', (

          (

@@ -588,9 +601,9 @@ 

              []

          ),

      ))

-     @patch('module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session')

-     def test_get_filtered_rpms_on_self_dep(self, get_session, br_filtered_rpms, expected):

-         session = MagicMock()

+     @patch('koji.ClientSession')

+     def test_get_filtered_rpms_on_self_dep(self, ClientSession, br_filtered_rpms, expected):

+         session = ClientSession.return_value

          session.listTaggedRPMS.return_value = (

              [

                  {

@@ -633,11 +646,11 @@ 

                  }

              ]

          )

-         get_session.return_value = session

          reuse_component_init_data()

          current_module = module_build_service.models.ModuleBuild.query.get(3)

          rv = KojiModuleBuilder._get_filtered_rpms_on_self_dep(current_module, br_filtered_rpms)

          assert set(rv) == set(expected)

+         session.assert_not_called()

  

      @pytest.mark.parametrize('cg_enabled,cg_devel_enabled', [

          (False, False),

@@ -666,6 +679,18 @@ 

          else:

              mock_koji_cg.koji_import.assert_not_called()

  

+     @patch('koji.ClientSession')

+     def test_get_anonymous_session(self, ClientSession):

+         mbs_config = mock.Mock(koji_profile='koji', koji_config='conf/koji.conf')

+         session = KojiModuleBuilder.get_session(mbs_config, 'someone', login=False)

+         assert ClientSession.return_value == session

+         assert ClientSession.return_value.krb_login.assert_not_called

+ 

+     @patch('koji.ClientSession')

+     def test_ensure_builder_use_a_logged_in_koji_session(self, ClientSession):

+         builder = KojiModuleBuilder('owner', MagicMock(), conf, 'module-tag', [])

+         builder.koji_session.krb_login.assert_called_once()

+ 

  

  class TestGetDistTagSRPM:

      """Test KojiModuleBuilder.get_disttag_srpm"""

file modified
+50 -29

@@ -31,7 +31,7 @@ 

  import module_build_service.scheduler.handlers.repos # noqa

  from module_build_service import models, conf, build_logs, Modulemd, glib

  

- from mock import patch, Mock, MagicMock, call, mock_open

+ from mock import patch, Mock, call, mock_open

  import kobo.rpmlib

  

  from tests import init_data

@@ -56,6 +56,13 @@ 

          module.cg_build_koji_tag = "f27-module-candidate"

          self.cg = KojiContentGenerator(module, conf)

  

+         self.p_read_config = patch('koji.read_config', return_value={

+             'authtype': 'kerberos',

+             'timeout': 60,

+             'server': 'http://koji.example.com/'

+         })

+         self.mock_read_config = self.p_read_config.start()

+ 

          # Ensure that there is no build log from other tests

          try:

              file_path = build_logs.path(self.cg.module)

@@ -64,6 +71,8 @@ 

              pass

  

      def teardown_method(self, test_method):

+         self.p_read_config.stop()

+ 

          # Necessary to restart the twisted reactor for the next test.

          import sys

          del sys.modules['twisted.internet.reactor']

@@ -76,7 +85,7 @@ 

          except OSError:

              pass

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

+     @patch("koji.ClientSession")

      @patch("subprocess.Popen")

      @patch("subprocess.check_output", return_value='1.4')

      @patch("pkg_resources.get_distribution")

@@ -86,12 +95,11 @@ 

             "_koji_rpms_in_tag"))

      @pytest.mark.parametrize("devel", (False, True))

      def test_get_generator_json(self, rpms_in_tag, machine, distro, pkg_res, coutput, popen,

-                                 get_session, devel):

+                                 ClientSession, devel):

          """ Test generation of content generator json """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.return_value = {"arches": ""}

-         get_session.return_value = koji_session

          distro.return_value = ("Fedora", "25", "Twenty Five")

          machine.return_value = "i686"

          pkg_res.return_value = Mock()

@@ -131,7 +139,10 @@ 

              assert ret["build"]["name"] == "nginx-devel"

              assert ret["build"]["extra"]["typeinfo"]["module"]["name"] == "nginx-devel"

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

+         # Ensure an anonymous Koji session works

+         koji_session.krb_login.assert_not_called()

+ 

+     @patch("koji.ClientSession")

      @patch("subprocess.Popen")

      @patch("subprocess.check_output", return_value='1.4')

      @patch("pkg_resources.get_distribution")

@@ -140,12 +151,11 @@ 

      @patch(("module_build_service.builder.KojiContentGenerator.KojiContentGenerator."

             "_koji_rpms_in_tag"))

      def test_get_generator_json_no_log(self, rpms_in_tag, machine, distro, pkg_res, coutput, popen,

-                                        get_session):

+                                        ClientSession):

          """ Test generation of content generator json """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.return_value = {"arches": ""}

-         get_session.return_value = koji_session

          distro.return_value = ("Fedora", "25", "Twenty Five")

          machine.return_value = "i686"

          pkg_res.return_value = Mock()

@@ -174,6 +184,9 @@ 

          rpms_in_tag.assert_called_once()

          assert expected_output == ret

  

+         # Anonymous koji session should work well.

+         koji_session.krb_login.assert_not_called()

+ 

      def test_prepare_file_directory(self):

          """ Test preparation of directory with output files """

          dir_path = self.cg._prepare_file_directory()

@@ -193,26 +206,27 @@ 

          with open(path.join(dir_path, "modulemd.i686.txt")) as mmd:

              assert len(mmd.read()) == 255

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

-     def test_tag_cg_build(self, get_session):

+     @patch("koji.ClientSession")

+     def test_tag_cg_build(self, ClientSession):

          """ Test that the CG build is tagged. """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.return_value = {'id': 123}

-         get_session.return_value = koji_session

  

          self.cg._tag_cg_build()

  

          koji_session.getTag.assert_called_once_with(self.cg.module.cg_build_koji_tag)

          koji_session.tagBuild.assert_called_once_with(123, "nginx-0-2.10e50d06")

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

-     def test_tag_cg_build_fallback_to_default_tag(self, get_session):

+         # tagBuild requires logging into a session in advance.

+         koji_session.krb_login.assert_called_once()

+ 

+     @patch("koji.ClientSession")

+     def test_tag_cg_build_fallback_to_default_tag(self, ClientSession):

          """ Test that the CG build is tagged to default tag. """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.side_effect = [{}, {'id': 123}]

-         get_session.return_value = koji_session

  

          self.cg._tag_cg_build()

  

@@ -221,30 +235,35 @@ 

              call(conf.koji_cg_default_build_tag)]

          koji_session.tagBuild.assert_called_once_with(123, "nginx-0-2.10e50d06")

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

-     def test_tag_cg_build_no_tag_set(self, get_session):

+         # tagBuild requires logging into a session in advance.

+         koji_session.krb_login.assert_called_once()

+ 

+     @patch("koji.ClientSession")

+     def test_tag_cg_build_no_tag_set(self, ClientSession):

          """ Test that the CG build is not tagged when no tag set. """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.side_effect = [{}, {'id': 123}]

-         get_session.return_value = koji_session

  

          self.cg.module.cg_build_koji_tag = None

          self.cg._tag_cg_build()

  

          koji_session.tagBuild.assert_not_called()

+         # tagBuild requires logging into a session in advance.

+         koji_session.krb_login.assert_called_once()

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

-     def test_tag_cg_build_no_tag_available(self, get_session):

+     @patch("koji.ClientSession")

+     def test_tag_cg_build_no_tag_available(self, ClientSession):

          """ Test that the CG build is not tagged when no tag available. """

-         koji_session = MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.side_effect = [{}, {}]

-         get_session.return_value = koji_session

  

          self.cg._tag_cg_build()

  

          koji_session.tagBuild.assert_not_called()

+         # tagBuild requires logging into a session in advance.

+         koji_session.krb_login.assert_called_once()

  

      @patch("module_build_service.builder.KojiContentGenerator.open", create=True)

      def test_get_arch_mmd_output(self, patched_open):

@@ -313,9 +332,9 @@ 

              'type': 'file'

          }

  

-     @patch("module_build_service.builder.KojiContentGenerator.get_session")

-     def test_koji_rpms_in_tag(self, get_session):

-         koji_session = MagicMock()

+     @patch("koji.ClientSession")

+     def test_koji_rpms_in_tag(self, ClientSession):

+         koji_session = ClientSession.return_value

          koji_session.getUser.return_value = GET_USER_RV

          koji_session.getTag.return_value = {"arches": "x86_64"}

  

@@ -379,7 +398,6 @@ 

               [{'license': 'MIT'}],

               [{'license': 'GPL'}]]

          ]

-         get_session.return_value = koji_session

  

          rpms = self.cg._koji_rpms_in_tag("tag")

          for rpm in rpms:

@@ -391,6 +409,9 @@ 

                  assert rpm["exclusivearch"] == ["x86_64"]

                  assert rpm["license"] == "GPL"

  

+         # Listing tagged RPMs does not require to log into a session

+         koji_session.krb_login.assert_not_called()

+ 

      def _add_test_rpm(self, nevra, srpm_name=None, multilib=None,

                        koji_srpm_name=None, excludearch=None, exclusivearch=None,

                        license=None):

@@ -88,12 +88,11 @@ 

  

      @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",

             return_value={'build': [], 'srpm-build': []})

-     @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

      @patch("module_build_service.builder.GenericBuilder.create_from_module")

      @patch('module_build_service.resolver.DBResolver')

      @patch('module_build_service.resolver.GenericResolver')

      def test_new_repo_called_when_macros_reused(

-             self, generic_resolver, resolver, create_builder, koji_get_session, dbg):

+             self, generic_resolver, resolver, create_builder, dbg):

          """

          Test that newRepo is called when module-build-macros build is reused.

          """

@@ -101,7 +100,6 @@ 

              scheduler_init_data()

              koji_session = mock.MagicMock()

              koji_session.newRepo.return_value = 123456

-             koji_get_session.return_value = koji_session

  

              builder = mock.MagicMock()

              builder.koji_session = koji_session

@@ -130,12 +128,11 @@ 

  

      @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",

             return_value={'build': [], 'srpm-build': []})

-     @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

      @patch("module_build_service.builder.GenericBuilder.create_from_module")

      @patch('module_build_service.resolver.DBResolver')

      @patch('module_build_service.resolver.GenericResolver')

      def test_new_repo_not_called_when_macros_not_reused(

-             self, generic_resolver, resolver, create_builder, koji_get_session, dbg):

+             self, generic_resolver, resolver, create_builder, dbg):

          """

          Test that newRepo is called everytime for module-build-macros

          """

@@ -143,7 +140,6 @@ 

              scheduler_init_data()

              koji_session = mock.MagicMock()

              koji_session.newRepo.return_value = 123456

-             koji_get_session.return_value = koji_session

  

              builder = mock.MagicMock()

              builder.koji_session = koji_session

@@ -166,12 +162,11 @@ 

  

      @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",

             return_value={'build': [], 'srpm-build': []})

-     @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

      @patch("module_build_service.builder.GenericBuilder.create_from_module")

      @patch('module_build_service.resolver.DBResolver')

      @patch('module_build_service.resolver.GenericResolver')

      def test_set_cg_build_koji_tag_fallback_to_default(

-             self, generic_resolver, resolver, create_builder, koji_get_session, dbg):

+             self, generic_resolver, resolver, create_builder, dbg):

          """

          Test that build.cg_build_koji_tag fallbacks to default tag.

          """

@@ -183,7 +178,6 @@ 

              scheduler_init_data()

              koji_session = mock.MagicMock()

              koji_session.newRepo.return_value = 123456

-             koji_get_session.return_value = koji_session

  

              builder = mock.MagicMock()

              builder.koji_session = koji_session

@@ -213,14 +207,13 @@ 

      ])

      @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",

             return_value={'build': [], 'srpm-build': []})

-     @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

      @patch("module_build_service.builder.GenericBuilder.create_from_module")

      @patch('module_build_service.resolver.DBResolver')

      @patch('module_build_service.resolver.GenericResolver')

      @patch("module_build_service.config.Config.base_module_names",

             new_callable=mock.PropertyMock, return_value=set(["base-runtime", "platform"]))

      def test_set_cg_build_koji_tag(

-             self, cfg, generic_resolver, resolver, create_builder, koji_get_session, dbg,

+             self, cfg, generic_resolver, resolver, create_builder, dbg,

              koji_cg_tag_build, expected_cg_koji_build_tag):

          """

          Test that build.cg_build_koji_tag is set.

@@ -233,7 +226,6 @@ 

              scheduler_init_data()

              koji_session = mock.MagicMock()

              koji_session.newRepo.return_value = 123456

-             koji_get_session.return_value = koji_session

  

              builder = mock.MagicMock()

              builder.koji_session = koji_session

@@ -32,21 +32,27 @@ 

  @patch("module_build_service.builder.GenericBuilder.default_buildroot_groups",

         return_value={'build': [], 'srpm-build': []})

  @patch("module_build_service.scheduler.consumer.get_global_consumer")

- @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

  @patch("module_build_service.builder.GenericBuilder.create_from_module")

  class TestPoller:

  

      def setup_method(self, test_method):

          reuse_component_init_data()

  

+         self.p_read_config = patch('koji.read_config', return_value={

+             'authtype': 'kerberos',

+             'timeout': 60,

+             'server': 'http://koji.example.com/'

+         })

+         self.mock_read_config = self.p_read_config.start()

+ 

      def teardown_method(self, test_method):

+         self.p_read_config.stop()

          clean_database()

  

      @pytest.mark.parametrize('fresh', [True, False])

      @patch('module_build_service.utils.batches.start_build_component')

      def test_process_paused_module_builds(self, start_build_component, create_builder,

-                                           koji_get_session, global_consumer,

-                                           dbg, fresh):

+                                           global_consumer, dbg, fresh):

          """

          Tests general use-case of process_paused_module_builds.

          """

@@ -54,9 +60,6 @@ 

          consumer.incoming = queue.Queue()

          global_consumer.return_value = consumer

  

-         koji_session = mock.MagicMock()

-         koji_get_session.return_value = koji_session

- 

          builder = mock.MagicMock()

          create_builder.return_value = builder

  

@@ -95,9 +98,9 @@ 

  

          assert len(start_build_component.mock_calls) == expected_build_calls

  

-     def test_trigger_new_repo_when_failed(self, create_builder,

-                                           koji_get_session, global_consumer,

-                                           dbg):

+     @patch("koji.ClientSession")

+     def test_trigger_new_repo_when_failed(

+             self, ClientSession, create_builder, global_consumer, dbg):

          """

          Tests that we call koji_sesion.newRepo when newRepo task failed.

          """

@@ -105,11 +108,10 @@ 

          consumer.incoming = queue.Queue()

          global_consumer.return_value = consumer

  

-         koji_session = mock.MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getTag = lambda tag_name: {'name': tag_name}

          koji_session.getTaskInfo.return_value = {'state': koji.TASK_STATES['FAILED']}

          koji_session.newRepo.return_value = 123456

-         koji_get_session.return_value = koji_session

  

          builder = mock.MagicMock()

          builder.buildroot_ready.return_value = False

@@ -129,9 +131,9 @@ 

          koji_session.newRepo.assert_called_once_with(

              "module-testmodule-master-20170219191323-c40c156c-build")

  

-     def test_trigger_new_repo_when_succeded(self, create_builder,

-                                             koji_get_session, global_consumer,

-                                             dbg):

+     @patch('koji.ClientSession')

+     def test_trigger_new_repo_when_succeeded(

+             self, ClientSession, create_builder, global_consumer, dbg):

          """

          Tests that we do not call koji_sesion.newRepo when newRepo task

          succeeded.

@@ -140,11 +142,10 @@ 

          consumer.incoming = queue.Queue()

          global_consumer.return_value = consumer

  

-         koji_session = mock.MagicMock()

+         koji_session = ClientSession.return_value

          koji_session.getTag = lambda tag_name: {'name': tag_name}

          koji_session.getTaskInfo.return_value = {'state': koji.TASK_STATES['CLOSED']}

          koji_session.newRepo.return_value = 123456

-         koji_get_session.return_value = koji_session

  

          builder = mock.MagicMock()

          builder.buildroot_ready.return_value = False

@@ -169,7 +170,7 @@ 

          assert module_build.new_repo_task_id == 0

  

      def test_process_paused_module_builds_waiting_for_repo(

-             self, create_builder, koji_get_session, global_consumer, dbg):

+             self, create_builder, global_consumer, dbg):

          """

          Tests that process_paused_module_builds does not start new batch

          when we are waiting for repo.

@@ -178,9 +179,6 @@ 

          consumer.incoming = queue.Queue()

          global_consumer.return_value = consumer

  

-         koji_session = mock.MagicMock()

-         koji_get_session.return_value = koji_session

- 

          builder = mock.MagicMock()

          create_builder.return_value = builder

  

@@ -205,72 +203,161 @@ 

          for component in components:

              assert component.state is None

  

-     def test_delete_old_koji_targets(

-             self, create_builder, koji_get_session, global_consumer, dbg):

-         """

-         Tests that we delete koji target when time_completed is older than

-         koji_target_delete_time value.

-         """

+     @patch('koji.ClientSession')

+     def test_old_build_targets_are_not_associated_with_any_module_builds(

+             self, ClientSession, create_builder, global_consumer, dbg):

          consumer = mock.MagicMock()

          consumer.incoming = queue.Queue()

          global_consumer.return_value = consumer

  

-         for state_name, state in models.BUILD_STATES.items():

-             koji_session = mock.MagicMock()

-             koji_session.getBuildTargets.return_value = [

-                 {'dest_tag_name': 'module-tag', 'id': 852, 'name': 'module-tag'},

-                 {'dest_tag_name': 'f26', 'id': 853, 'name': 'f26'},

-                 {'dest_tag_name': 'module-tag2', 'id': 853, 'name': 'f26'}]

-             koji_get_session.return_value = koji_session

+         koji_session = ClientSession.return_value

+         # No created module build has any of these tags.

+         koji_session.getBuildTargets.return_value = [

+             {'dest_tag_name': 'module-xxx-1'},

+             {'dest_tag_name': 'module-yyy-2'},

+         ]

+ 

+         hub = mock.MagicMock()

+         poller = MBSProducer(hub)

+         poller.delete_old_koji_targets(conf, db.session)

  

-             builder = mock.MagicMock()

-             create_builder.return_value = builder

+         koji_session.deleteBuildTarget.assert_not_called()

  

-             # Change the batch to 2, so the module build is in state where

-             # it is not building anything, but the state is "build".

-             module_build = models.ModuleBuild.query.filter_by(id=3).one()

-             module_build.state = state

-             module_build.koji_tag = "module-tag"

-             module_build.time_completed = datetime.utcnow()

-             module_build.new_repo_task_id = 123456

-             db.session.commit()

+     @patch('koji.ClientSession')

+     def test_dont_delete_base_module_build_target(

+             self, ClientSession, create_builder, global_consumer, dbg):

+         module_build = models.ModuleBuild.query.filter_by(id=3).one()

+ 

+         koji_session = ClientSession.return_value

+         # No created module build has any of these tags.

+         koji_session.getBuildTargets.return_value = [

+             {'dest_tag_name': module_build.koji_tag},

+         ]

+ 

+         consumer = mock.MagicMock()

+         consumer.incoming = queue.Queue()

+         global_consumer.return_value = consumer

+ 

+         # If module build's name is one of base module names, build target

+         # should not be deleted.

+         with patch.object(conf, 'base_module_names', new=[module_build.name]):

  

-             # Poll :)

              hub = mock.MagicMock()

              poller = MBSProducer(hub)

              poller.delete_old_koji_targets(conf, db.session)

  

-             module_build = models.ModuleBuild.query.filter_by(id=3).one()

-             db.session.refresh(module_build)

-             module_build.time_completed = datetime.utcnow() - timedelta(hours=23)

-             db.session.commit()

-             poller.delete_old_koji_targets(conf, db.session)

+             koji_session.deleteBuildTarget.assert_not_called()

+ 

+     @patch('koji.ClientSession')

+     def test_dont_delete_build_target_for_unfinished_module_builds(

+             self, ClientSession, create_builder, global_consumer, dbg):

+         module_build = models.ModuleBuild.query.filter_by(id=3).one()

  

-             # deleteBuildTarget should not be called, because time_completed is

-             # set to "now".

-             assert not koji_session.deleteBuildTarget.called

+         koji_session = ClientSession.return_value

+         # No created module build has any of these tags.

+         koji_session.getBuildTargets.return_value = [

+             {'dest_tag_name': module_build.koji_tag},

+         ]

  

-             # Try removing non-modular target - should not happen

-             db.session.refresh(module_build)

-             module_build.koji_tag = "module-tag2"

-             module_build.time_completed = datetime.utcnow() - timedelta(hours=25)

+         consumer = mock.MagicMock()

+         consumer.incoming = queue.Queue()

+         global_consumer.return_value = consumer

+ 

+         # Each time when a module build is in one of these state, build target

+         # should not be deleted.

+         for state in ['init', 'wait', 'build']:

+             module_build.state = state

              db.session.commit()

+ 

+             hub = mock.MagicMock()

+             poller = MBSProducer(hub)

              poller.delete_old_koji_targets(conf, db.session)

-             assert not koji_session.deleteBuildTarget.called

  

-             # Refresh our module_build object and set time_completed 25 hours ago

-             db.session.refresh(module_build)

-             module_build.time_completed = datetime.utcnow() - timedelta(hours=25)

-             module_build.koji_tag = "module-tag"

-             db.session.commit()

+             koji_session.deleteBuildTarget.assert_not_called()

+ 

+     @patch('koji.ClientSession')

+     def test_only_delete_build_target_with_allowed_koji_tag_prefix(

+             self, ClientSession, create_builder, global_consumer, dbg):

+         module_build_2 = models.ModuleBuild.query.filter_by(id=2).one()

+         module_build_3 = models.ModuleBuild.query.filter_by(id=3).one()

+ 

+         # Only module build 1's build target should be deleted.

+         module_build_2.koji_tag = 'module-tag1'

+         module_build_2.state = models.BUILD_STATES['done']

+         # Ensure to exceed the koji_target_delete_time easily later for deletion

+         module_build_2.time_completed = datetime.utcnow() - timedelta(hours=24)

+         module_build_3.koji_tag = 'f28'

+         db.session.commit()

+         db.session.refresh(module_build_2)

+         db.session.refresh(module_build_3)

+ 

+         koji_session = ClientSession.return_value

+         # No created module build has any of these tags.

+         koji_session.getBuildTargets.return_value = [

+             {

+                 'id': 1,

+                 'dest_tag_name': module_build_2.koji_tag,

+                 'name': module_build_2.koji_tag

+             },

+             {

+                 'id': 2,

+                 'dest_tag_name': module_build_3.koji_tag,

+                 'name': module_build_3.koji_tag

+             },

+         ]

+ 

+         consumer = mock.MagicMock()

+         consumer.incoming = queue.Queue()

+         global_consumer.return_value = consumer

  

+         with patch.object(conf, 'koji_tag_prefixes',

+                           new=['module', 'another-prefix']):

+             with patch.object(conf, 'koji_target_delete_time', new=60):

+                 hub = mock.MagicMock()

+                 poller = MBSProducer(hub)

+                 poller.delete_old_koji_targets(conf, db.session)

+ 

+             koji_session.deleteBuildTarget.assert_called_once_with(1)

+             koji_session.krb_login.assert_called_once()

+ 

+     @patch('koji.ClientSession')

+     def test_cant_delete_build_target_if_not_reach_delete_time(

+             self, ClientSession, create_builder, global_consumer, dbg):

+         module_build_2 = models.ModuleBuild.query.filter_by(id=2).one()

+ 

+         # Only module build 1's build target should be deleted.

+         module_build_2.koji_tag = 'module-tag1'

+         module_build_2.state = models.BUILD_STATES['done']

+         # Ensure to exceed the koji_target_delete_time easily later for deletion

+         module_build_2.time_completed = datetime.utcnow() - timedelta(minutes=5)

+         db.session.commit()

+         db.session.refresh(module_build_2)

+ 

+         koji_session = ClientSession.return_value

+         # No created module build has any of these tags.

+         koji_session.getBuildTargets.return_value = [

+             {

+                 'id': 1,

+                 'dest_tag_name': module_build_2.koji_tag,

+                 'name': module_build_2.koji_tag

+             },

+         ]

+ 

+         consumer = mock.MagicMock()

+         consumer.incoming = queue.Queue()

+         global_consumer.return_value = consumer

+ 

+         with patch.object(conf, 'koji_tag_prefixes', new=['module']):

+             # Use default koji_target_delete_time in config. That time is long

+             # enough for test.

+             hub = mock.MagicMock()

+             poller = MBSProducer(hub)

              poller.delete_old_koji_targets(conf, db.session)

  

-             if state_name in ["done", "ready", "failed"]:

-                 koji_session.deleteBuildTarget.assert_called_once_with(852)

+             koji_session.deleteBuildTarget.assert_not_called()

  

-     def test_process_waiting_module_build(self, create_builder, koji_get_session,

-                                           global_consumer, dbg):

+     def test_process_waiting_module_build(

+             self, create_builder, global_consumer, dbg):

          """ Test that processing old waiting module builds works. """

  

          consumer = mock.MagicMock()

@@ -300,8 +387,8 @@ 

          # ensure the time_modified was changed.

          assert module_build.time_modified > original

  

-     def test_process_waiting_module_build_not_old_enough(self, create_builder, koji_get_session,

-                                                          global_consumer, dbg):

+     def test_process_waiting_module_build_not_old_enough(

+             self, create_builder, global_consumer, dbg):

          """ Test that we do not process young waiting builds. """

  

          consumer = mock.MagicMock()

@@ -329,8 +416,8 @@ 

          # Ensure we did *not* process the 9 minute-old build.

          assert consumer.incoming.qsize() == 0

  

-     def test_process_waiting_module_build_none_found(self, create_builder, koji_get_session,

-                                                      global_consumer, dbg):

+     def test_process_waiting_module_build_none_found(

+             self, create_builder, global_consumer, dbg):

          """ Test nothing happens when no module builds are waiting. """

  

          consumer = mock.MagicMock()

@@ -349,8 +436,8 @@ 

          # Ensure we did *not* process any of the non-waiting builds.

          assert consumer.incoming.qsize() == 0

  

-     def test_cleanup_stale_failed_builds(self, create_builder, koji_get_session,

-                                          global_consumer, dbg):

+     def test_cleanup_stale_failed_builds(

+             self, create_builder, global_consumer, dbg):

          """ Test that one of the two module builds gets to the garbage state when running

          cleanup_stale_failed_builds.

          """

@@ -402,8 +489,8 @@ 

              'module-build-macros-0.1-1.module+0+d027b723'

          ])

  

-     def test_cleanup_stale_failed_builds_no_components(self, create_builder, koji_get_session,

-                                                        global_consumer, dbg):

+     def test_cleanup_stale_failed_builds_no_components(

+             self, create_builder, global_consumer, dbg):

          """ Test that a module build without any components built gets to the garbage state when

          running cleanup_stale_failed_builds.

          """

@@ -445,8 +532,8 @@ 

  

      @pytest.mark.parametrize('test_state', [models.BUILD_STATES[state]

                                              for state in conf.cleanup_stuck_builds_states])

-     def test_cancel_stuck_module_builds(self, create_builder, koji_get_session, global_consumer,

-                                         dbg, test_state):

+     def test_cancel_stuck_module_builds(

+             self, create_builder, global_consumer, dbg, test_state):

  

          module_build1 = models.ModuleBuild.query.get(1)

          module_build1.state = test_state

@@ -25,7 +25,7 @@ 

  

  import module_build_service.scm

  

- from mock import patch, PropertyMock, Mock

+ from mock import patch, PropertyMock

  from shutil import copyfile

  from os import path, mkdir

  from os.path import dirname

@@ -417,8 +417,8 @@ 

                  for key, part in zip(nsvc_keys, nsvc_parts):

                      assert item[key] == part

  

-     @patch("module_build_service.builder.KojiModuleBuilder.KojiModuleBuilder.get_session")

-     def test_query_builds_with_binary_rpm(self, mock_get_session):

+     @patch("koji.ClientSession")

+     def test_query_builds_with_binary_rpm(self, ClientSession):

          """

          Test for querying MBS with the binary rpm filename. MBS should return all the modules,

          which contain the rpm.

@@ -431,13 +431,17 @@ 

                       {"name": "non-module-tag"},

                       {"name": "module-testmodule-master-20170109091357-78e4a6fd"}]

  

-         mock_session = Mock()

+         mock_session = ClientSession.return_value

          mock_session.getRPM.return_value = mock_rpm_md

          mock_session.listTags.return_value = mock_tags

-         mock_get_session.return_value = mock_session

  

          rpm = quote('module-build-macros-0.1-1.testmodule_master_20170303190726.src.rpm')

-         rv = self.client.get('/module-build-service/1/module-builds/?rpm=%s' % rpm)

+         with patch('koji.read_config', return_value={

+             'authtype': 'kerberos',

+             'timeout': 60,

+             'server': 'http://koji.example.com/'

+         }):

+             rv = self.client.get('/module-build-service/1/module-builds/?rpm=%s' % rpm)

          results = json.loads(rv.data)['items']

  

          assert len(results) == 2

@@ -448,6 +452,8 @@ 

              "module-build-macros-0.1-1.testmodule_master_20170303190726.src.rpm")

          mock_session.listTags.assert_called_once_with(mock_rpm_md["build_id"])

  

+         mock_session.krb_login.assert_not_called()

+ 

      @patch('module_build_service.config.Config.system',

             new_callable=PropertyMock, return_value="invalid_builder")

      def test_query_builds_with_binary_rpm_not_koji(self, mock_builder):

MBS calls some read-only Koji APIs which does not require to log into a
session. This patch makes it optional to choose whether to login a
session and use anonymous session properly to call those read-only APIs.

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

rebased onto 55add5c

6 months ago

I'm missing any test for this functionality.

1 new commit added

  • Add tests for use of anonymous koji session
6 months ago

2 new commits added

  • Add tests for use of anonymous koji session
  • Use anonymous Koji session properly
6 months ago

It seems like owner is no longer used. Do you mind removing this parameter since you're modifying this anyways? If you don't have time, we can make this a separate PR.

You could avoid this indentation by just doing:

if not login:
    return koji_session

authtype = koji_config.authtype
...

@cqi looks great

Commit 692c34f fixes this pull-request

Pull-Request has been merged by mprahl

6 months ago

Pull-Request has been merged by mprahl

6 months ago