Merge pull request #2766 from coolo/split_source_checker

Refactoring and tests in check_source

check_source.pl

@@ -17,24 +17,6 @@ my $old = $ARGV[0];
 my $dir = $ARGV[1];
 my $bname = basename($dir);
 
-if (-f "$dir/_service") {
-    my $service = XMLin("$dir/_service", ForceArray => ['service']);
-    while( my ($name, $s) = each %{$service->{service}} ) {
-        my $mode = $s->{mode} || '';
-        next if ($mode eq "localonly" || $mode eq "disabled" || $mode eq "buildtime" || $mode eq "manual" );
-        print "Services are only allowed if they are mode='localonly', 'disabled', 'manual' or 'buildtime'. Please change the mode of $name and use `osc service localrun/disabledrun`.\n";
-        $ret = 1;
-    }
-    # move it away to have full service from source validator
-    rename("$dir/_service", "$dir/_service.bak") || die "rename failed";
-}
-
-for my $file (glob("$dir/_service:*")) {
-    $file=basename($file);
-    print "Found _service generated file $file in checkout. Please clean this up first.";
-    $ret = 1;
-}
-
 my @specs = map basename($_), glob("$dir/*.spec");
 
 if (@specs) {
@@ -240,64 +222,11 @@ if (-d "$old") {
     }
 }
 
-my $odir2 = getcwd;
 my $tmpdir = tempdir("obs-XXXXXXX", TMPDIR => 1, CLEANUP => 1);
 chdir($dir) || die 'tempdir failed';
 if (system("/usr/lib/obs/service/download_files","--enforceupstream", "yes", "--enforcelocal", "yes", "--outdir", $tmpdir)) {
     print "Source URLs are not valid. Try \"osc service runall download_files\".\n";
     $ret = 2;
 }
-chdir($odir2);
 
-for my $rpmlint (glob("$dir/*rpmlintrc")) {
-    open(RPMLINTRC, $rpmlint);
-    while (<RPMLINTRC>) {
-        if (m/^\s*setBadness/) {
-            print "For Factory submissions, you cannot use setBadness. Use filters in $rpmlint.\n";
-            $ret = 1;
-        }
-    }
-}
-
-exit($ret) if $ret;
-
-# now check if the change is small enough to warrent a review-by-mail
-exit(0) unless -d $old;
-
-sub prepare_package($) {
-
-    my $files = shift;
-
-    unlink glob "*.changes"; # ignore changes
-    unlink glob "*.tar.*"; # we can't diff them anyway
-    unlink glob "*.zip";
-
-    # restore original spec file
-    for my $spec (glob("*.beforeurlstrip")) {
-        my $oldname = $spec;
-        $oldname =~ s/.beforeurlstrip//;
-        rename($spec, $oldname);
-    }
-
-    for my $spec (glob("*.spec")) {
-        open(SPEC, "/usr/lib/obs/service/format_spec_file.files/prepare_spec $spec | grep -v '^#' |");
-        my @lines = <SPEC>;
-        close(SPEC);
-        open(SPEC, ">", $spec);
-        print SPEC join('', @lines);
-        close(SPEC);
-    }
-}
-
-# move it back so we also diff the service file
-if (-f "$dir/_service.bak") {
-    rename("$dir/_service.bak", "$dir/_service") || die "rename failed";
-}
-
-my %files;
-chdir($old);
-prepare_package(\%files);
-chdir($odir2);
-chdir($dir);
-prepare_package(\%files);
-exit(0);
+exit($ret);

check_source.py

@@ -1,5 +1,6 @@
 #!/usr/bin/python3
 
+import glob
 import os
 import re
 import shutil
@@ -48,7 +49,6 @@ class CheckSource(ReviewBot.ReviewBot):
         self.review_team = config.get('review-team')
         self.mail_release_list = config.get('mail-release-list')
         self.staging_group = config.get('staging-group')
-        self.repo_checker = config.get('repo-checker')
         self.required_maintainer = config.get('required-source-maintainer', '')
         self.devel_whitelist = config.get('devel-whitelist', '').split()
         self.skip_add_reviews = False
@@ -208,6 +208,12 @@ class CheckSource(ReviewBot.ReviewBot):
                 target_package, target_package, new_info['name'])
             return False
 
+        if not self.check_service_file(target_package):
+            return False
+
+        if not self.check_rpmlint(target_package):
+            return False
+
         # Run check_source.pl script and interpret output.
         source_checker = os.path.join(CheckSource.SCRIPT_PATH, 'check_source.pl')
         civs = ''
@@ -253,8 +259,6 @@ class CheckSource(ReviewBot.ReviewBot):
                                                      message='skipping the staging process since only .changes modifications')
                 else:
                     self.logger.debug('unable to skip staging review since not a member of staging group')
-            elif self.repo_checker is not None:
-                self.add_review(self.request, by_user=self.repo_checker, msg='Please review build success')
 
         return True
 
@@ -269,6 +273,42 @@ class CheckSource(ReviewBot.ReviewBot):
         result = osc.core.search(self.apiurl, **search)
         return result['package'].attrib['matches'] != '0'
 
+    def check_service_file(self, directory):
+        ALLOWED_MODES = ['localonly', 'disabled', 'buildtime', 'manual']
+
+        servicefile = os.path.join(directory, '_service')
+        if os.path.exists(servicefile):
+            services = ET.parse(servicefile)
+            for service in services.findall('service'):
+                mode = service.get('mode')
+                if mode in ALLOWED_MODES:
+                    continue
+                allowed = ', '.join(ALLOWED_MODES)
+                name = service.get('name')
+                self.review_messages[
+                    'declined'] = f"Services are only allowed if their mode is one of {allowed}. " + \
+                    f"Please change the mode of {name} and use `osc service localrun/disabledrun`."
+                return False
+            # remove it away to have full service from source validator
+            os.unlink(servicefile)
+
+        for file in glob.glob(os.path.join(directory, "_service:*")):
+            file = os.path.basename(file)
+            self.review_messages['declined'] = f"Found _service generated file {file} in checkout. Please clean this up first."
+            return False
+
+        return True
+
+    def check_rpmlint(self, directory):
+        for rpmlintrc in glob.glob(os.path.join(directory, "*rpmlintrc")):
+            with open(rpmlintrc, 'r') as f:
+                for line in f:
+                    if not re.match(r'^\s*setBadness', line):
+                        continue
+                    self.review_messages['declined'] = f"For product submissions, you cannot use setBadness. Use filters in {rpmlintrc}."
+                    return False
+        return True
+
     def source_has_correct_maintainers(self, source_project):
         """Checks whether the source project has the required maintainer
 
@@ -413,9 +453,6 @@ class CheckSource(ReviewBot.ReviewBot):
         if not self.ignore_devel:
             self.devel_project_review_ensure(request, action.tgt_project, action.tgt_package)
 
-        if not self.skip_add_reviews and self.repo_checker is not None:
-            self.add_review(self.request, by_user=self.repo_checker, msg='Is this delete request safe?')
-
         return True
 
     def check_linked_package(self, action, linked):

dist/ci/docker-compose.yml

@@ -28,7 +28,7 @@ services:
       - cache
       - srcserver
       - repserver
-      - servicedispatch
+      - serviceserver
     ports:
       - "0.0.0.0:${OSRT_EXPOSED_OBS_PORT:-3000}:3000"
   srcserver:
@@ -37,9 +37,9 @@ services:
   repserver:
     <<: *obs
     command: chroot --userspec=obsrun / /usr/lib/obs/server/bs_repserver
-  servicedispatch:
+  serviceserver:
     <<: *obs
-    command: chroot --userspec=obsrun / /usr/lib/obs/server/bs_servicedispatch
+    command: /usr/lib/obs/server/bs_service
   smtp:
     <<: *testenv
     command: python3 /code/dist/ci/smtp/eml-server.py

dist/ci/miniobs/Dockerfile

@@ -6,7 +6,8 @@ RUN zypper ar http://download.opensuse.org/repositories/OBS:/Server:/Unstable/15
 
 RUN zypper install -y obs-api obs-worker obs-server \
     ca-certificates patch vim vim-data psmisc timezone \
-    glibc-locale aaa_base aaa_base-extras netcat net-tools
+    glibc-locale aaa_base aaa_base-extras netcat net-tools \
+    obs-service-recompress obs-service-format_spec_file
 
 COPY database.yml.local /srv/www/obs/api/config/database.yml
 

osclib/conf.py

@@ -36,8 +36,6 @@ DEFAULT = {
         'devel-project-enforce': 'True',
         'review-team': 'opensuse-review-team',
         'legal-review-group': 'legal-auto',
-        'repo_checker-no-filter': 'True',
-        'repo_checker-package-comment-devel': 'True',
         'pkglistgen-product-family-include': 'openSUSE:Leap:N',
         'pkglistgen-locales-from': 'openSUSE.product.in',
         'mail-list': 'factory@lists.opensuse.org',
@@ -87,8 +85,6 @@ DEFAULT = {
         'check-source-single-action-require': 'True',
         # review-team optionally added by leaper.py.
         'repo_checker-arch-whitelist': 'x86_64',
-        'repo_checker-no-filter': 'True',
-        'repo_checker-package-comment-devel': 'True',
         # 16 hour staging window for follow-ups since lower throughput.
         'splitter-staging-age-max': '57600',
         # No special packages since they will pass through SLE first.
@@ -116,8 +112,6 @@ DEFAULT = {
     r'openSUSE:(?P<project>Leap:(?P<version>[\d.]+)?:Update)$': {
         'main-repo': 'standard',
         'repo_checker-arch-whitelist': 'x86_64',
-        'repo_checker-no-filter': 'True',
-        'repo_checker-package-comment-devel': 'True',
         'review-install-check': 'maintenance-installcheck',
         'review-openqa': 'qam-openqa',
     },

tests/OBSLocal.py

@@ -1039,6 +1039,10 @@ class Package(object):
             with open(os.path.join(path, filename), 'rb') as f:
                 self.create_commit(filename=filename, text=f.read())
 
+    def wait_services(self):
+        url = osc.core.makeurl(APIURL, ['source', self.project.name, self.name], {'cmd': 'waitservice'})
+        osc.core.http_POST(url)
+
 
 class Request(object):
     """This class represents a request in the local OBS instance used to test the release tools and

tests/check_source_tests.py

@@ -234,6 +234,40 @@ class TestCheckSource(OBSLocal.TestCase):
         self.assertEqual(add_role_req.actions[0].tgt_project, SRC_PROJECT)
         self.assertEqual('Created automatically from request %s' % req.reqid, add_role_req.description)
 
+    @pytest.mark.usefixtures("default_config")
+    def test_bad_rpmlintrc(self):
+        """Declines a request if it uses setBadness in rpmlintrc"""
+        self._setup_devel_project(devel_files='blowfish-with-rpmlintrc')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+                                               self.devel_package.name, add_commit=False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        review = self.assertReview(req_id, by_user=(self.bot_user, 'declined'))
+        self.assertEqual('For product submissions, you cannot use setBadness. Use filters in blowfish/blowfish-rpmlintrc.', review.comment)
+
+    @pytest.mark.usefixtures("default_config")
+    @pytest.mark.skip(reason="Need the service in miniobs container first")
+    def test_remote_service(self):
+        """Declines _service files with remote services"""
+        self._setup_devel_project(devel_files='blowfish-with-remote-service')
+
+        req_id = self.wf.create_submit_request(self.devel_package.project,
+                                               self.devel_package.name, add_commit=False).reqid
+
+        self.assertReview(req_id, by_user=(self.bot_user, 'new'))
+
+        self.review_bot.set_request_ids([req_id])
+        self.review_bot.check_requests()
+
+        review = self.assertReview(req_id, by_user=(self.bot_user, 'declined'))
+        self.assertEqual('Services are only allowed if their mode is one of localonly, disabled, buildtime, ' +
+                         'manual. Please change the mode of recompress and use `osc service localrun/disabledrun`.', review.comment)
+
     def _setup_devel_project(self, maintainer={}, devel_files='blowfish-with-patch-changes',
                              target_files='blowfish'):
         devel_project = self.wf.create_project(SRC_PROJECT, maintainer=maintainer)
@@ -241,6 +275,7 @@ class TestCheckSource(OBSLocal.TestCase):
 
         fixtures_path = os.path.join(FIXTURES, 'packages', devel_files)
         self.devel_package.commit_files(fixtures_path)
+        self.devel_package.wait_services()
 
         fixtures_path = os.path.join(FIXTURES, 'packages', target_files)
         self.target_package = OBSLocal.Package('blowfish', self.wf.projects[PROJECT], devel_project=SRC_PROJECT)

tests/fixtures/packages/blowfish-with-remote-service/_service

@@ -0,0 +1,6 @@
+<services>
+  <service name="recompress">
+    <param name="file">*.tar</param>
+  </service>
+</services>
+

tests/fixtures/packages/blowfish-with-remote-service/blowfish.changes

@@ -0,0 +1,10 @@
+-------------------------------------------------------------------
+Wed Sep 22 07:30:53 UTC 2021 - Josef Reidinger <jreidinger@suse.com>
+
+- just add patch without explicitly mentioning it
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-remote-service/blowfish.spec

@@ -0,0 +1,20 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         blowfish-1.tar.gz
+Patch1:        test.patch
+BuildArch:      noarch
+
+%prep
+%patch1
+
+%changelog

tests/fixtures/packages/blowfish-with-remote-service/test.patch

@@ -0,0 +1 @@
++-

tests/fixtures/packages/blowfish-with-rpmlintrc/blowfish-rpmlintrc

@@ -0,0 +1 @@
+setBadness('script-without-shebang', 2)

tests/fixtures/packages/blowfish-with-rpmlintrc/blowfish.changes

@@ -0,0 +1,10 @@
+-------------------------------------------------------------------
+Wed Sep 22 07:30:53 UTC 2021 - Josef Reidinger <jreidinger@suse.com>
+
+- just add patch without explicitly mentioning it
+
+-------------------------------------------------------------------
+Thu Jul  8 07:36:30 UTC 2021 - Fisherman <fisherman@opensuse.org>
+
+- Initial version.
+- 1

tests/fixtures/packages/blowfish-with-rpmlintrc/blowfish.spec

@@ -0,0 +1,20 @@
+#
+# Copyright (c) 2020 SUSE LLC
+#
+# This file is under MIT license
+
+
+Name:           blowfish
+Version:        1
+Release:        0
+Summary:        Blowfish
+License:        GPL-2.0-only
+URL:            https://github.com/openSUSE/cockpit-wicked
+Source:         blowfish-1.tar.gz
+Patch1:        test.patch
+BuildArch:      noarch
+
+%prep
+%patch1
+
+%changelog

tests/fixtures/packages/blowfish-with-rpmlintrc/test.patch

@@ -0,0 +1 @@
++-