#1094 Use anonymous Koji session properly
Merged 11 months ago by mprahl. Opened 11 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

11 months ago

I'm missing any test for this functionality.

1 new commit added

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

2 new commits added

  • Add tests for use of anonymous koji session
  • Use anonymous Koji session properly
11 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

11 months ago

Pull-Request has been merged by mprahl

11 months ago