pagure

Clone

#2314 changing logic order to avoid error with no ci. Fixes #2312
Merged 6 years ago by tflink. Opened 6 years ago by tflink.
tflink/pagure feature/2312-fix-pr-when-no-ci  into  master

file modified
+3 -3 
@@ -1134,7 +1134,7 @@ 
 

          # Send notification to the CI server, if the comment added was a
 

          # notification and the PR is still open and project is not private
 

          if notification and request.status == 'Open' \
 

-             and request.project.ci_hook and PAGURE_CI \
 

+             and PAGURE_CI and request.project.ci_hook\
 

                  and not request.project.private:
 

              REDIS.publish('pagure.ci', json.dumps({
 

                  'ci_type': request.project.ci_hook.ci_type,
 
@@ -1153,7 +1153,7 @@ 
 

  

      if trigger_ci and comment.strip().lower() in trigger_ci:
 

          # Send notification to the CI server
 

-         if REDIS and request.project.ci_hook and PAGURE_CI:
 

+         if REDIS and PAGURE_CI and request.project.ci_hook:
 

              REDIS.publish('pagure.ci', json.dumps({
 

                  'ci_type': request.project.ci_hook.ci_type,
 

                  'pr': request.to_json(public=True, with_comments=False)
 
@@ -1518,7 +1518,7 @@ 
 

      )
 

  

      # Send notification to the CI server
 

-     if REDIS and request.project.ci_hook and PAGURE_CI \
 

+     if REDIS and PAGURE_CI and request.project.ci_hook \
 

              and not request.project.private:
 

          REDIS.publish('pagure.ci', json.dumps({
 

              'ci_type': request.project.ci_hook.ci_type,

This is a simple change to short-circuit the if statement if pagure-ci is not configured. I can't run the unit tests right now but when I test this in a deployment, it solves the 500 problem when a PR is created in a pagure instance without pagure-ci configured

It might be good to write a test for this if you are later able to get the tests running. LGTM otherwise.

In pagure/lib/__init__.py there are many other lines using the same code, maybe same fix should be applied?

Besides, pingou fixed same error in https://pagure.io/pagure/pull-request/2023. Compare the two fixtures, which one do you prefer?
Edited 6 years ago by zhsj

In pagure/lib/init.py there are many other lines using the same code, maybe same fix should be applied?

Yeah, it probably should. I'll update the PR.

Besides, pingou fixed same error in https://pagure.io/pagure/pull-request/2023. Compare the two fixtures, which one do you prefer?

That was merged 2 months ago and is running in my instance but I'm still hitting this error. Either something isn't set up right on my end or it wasn't a 100% fix. Are there other non-test instances running without pagure-ci?

1 new commit added

  • changing order for other PAGURE_CI comparisons
6 years ago

Ok so this is a case of import chain.

Basically, we have:

pagure/api/ci/jenkins.py:import pagure.lib.lib_ci as lib_ci

and then we have:

pagure/lib/lib_ci.py:from pagure.hooks import pagure_ci  # noqa: E402,F401

So the combination of the two should fixes the missing ci_hook attribute, but the trick is that the import of pagure.api.ci.jenkins at:

pagure/api/__init__.py:    from pagure.api.ci import jenkins  # noqa: E402

is conditioned by:

if pagure.APP.config.get('PAGURE_CI_SERVICES', False):

So if there are no PAGURE_CI_SERVICES defined the import chain is busted, thus the error that @tflink is seeing.

Going to merge this PR manually then, thanks @tflink !

Commit ad88b33 fixes this pull-request

Pull-Request has been merged by tflink@fedoraproject.org
6 years ago
Changes Summary 1
+3 -3
file changed
pagure/lib/__init__.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.