Differential D1045

Modify fedmsg Imports
AbandonedPublic
Actions

Authored by mprahl on Nov 4 2016, 6:15 PM.

Details

Reviewers
tflink
jskladan
Summary

This moves the location of the fedmsg import statements so that it is only imported when it is turned on.
This is beneficial for users who do not use fedmsg but use ResultsDB.

Test Plan

NA

Diff Detail

Repository
rRSDB resultsdb
Branch
fedmsg-imports (branched from develop)
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 906
Build 906: arc lint + arc unit
mprahl retitled this revision from to Modify fedmsg Imports.Nov 4 2016, 6:15 PM
mprahl updated this object.
mprahl edited the test plan for this revision. (Show Details)
mprahl added reviewers: tflink, jskladan.
mprahl added a subscriber: • ralph2.
mprahl added a comment.Nov 4 2016, 6:21 PM

The unit tests pass and the app runs in Docker

jskladan requested changes to this revision.Nov 6 2016, 9:10 AM

I'm not a huge fan of imports needlessly being in the middle of the code. This would IMO work equally if the conditional import was (e.g.) just before the QUERY_LIMIT in the "header" part.

Also - what is the actual benefit of doing that? I'm almost certain that it's not "doing one less import, just for the fun of it", but fail to see the actual use-case.

This revision now requires changes to proceed.Nov 6 2016, 9:10 AM
mprahl updated this revision to Diff 2671.Nov 7 2016, 2:10 PM

Address feedback

mprahl updated this revision to Diff 2672.Nov 7 2016, 2:11 PM
  • Address feedback
mprahl added a comment.Nov 7 2016, 2:15 PM

@jskladan I believe I addressed your concern in the latest commit. Please let me know your thoughts.

The reason why I created this PR is that we are in the process of deploying ResultsDB as a standalone service, but we will not be using fedmsg. By making the import conditional, it allows us to avoid installing fedmsg on our servers since it isn't needed.

jskladan accepted this revision.Nov 7 2016, 8:45 PM

Code looks good. Thanks!

In D1045#19608, @mprahl wrote:

... By making the import conditional, it allows us to avoid installing fedmsg on our servers since it isn't needed.

OK, but fedmsg package is required by the resultsdb package, so this would need specfile changes as well, right? Because as it is, the fedmsg won't be imported, but the dependency is still there.

This revision is now accepted and ready to land.Nov 7 2016, 8:45 PM
mprahl added a comment.Nov 7 2016, 8:48 PM

@jskladan you're right on the spec file change, but as of now I will resort to modifying it locally for internal builds until @tflink and I come together on something that works for both upstream and for internal.

jskladan added a comment.Nov 7 2016, 8:49 PM

Awesome, thanks for the info. That was the bit that did not make sense to me :)

kparal added a subscriber: kparal.Jan 24 2017, 4:10 PM

@jskladan: bleep, bloop, stale review warning!

mprahl added a comment.Jan 24 2017, 8:49 PM

We can close this review. If I still need this later on, I'll rebase and resubmit.

mprahl abandoned this revision.Jan 26 2017, 10:27 AM

Revision Contents

Diff 2672

View Options

resultsdb/controllers/api_v1.py

Show All 27 Lines
28from sqlalchemy import exc as sqlalchemy_exc 28​from sqlalchemy import exc as sqlalchemy_exc
29from werkzeug.exceptions import HTTPException 29​from werkzeug.exceptions import HTTPException
30 30​
31 31​
32# removed in flask 0.10 from flask.exceptions import JSONBadRequest 32​# removed in flask 0.10 from flask.exceptions import JSONBadRequest
33from werkzeug.exceptions import BadRequest as JSONBadRequest 33​from werkzeug.exceptions import BadRequest as JSONBadRequest
34 34​
35import iso8601 35​import iso8601
36import fedmsg ​
37 36​
38from resultsdb import app, db 37​from resultsdb import app, db
39from resultsdb.serializers.api_v1 import Serializer 38​from resultsdb.serializers.api_v1 import Serializer
40from resultsdb.models.results import Group, Result, Testcase, ResultData 39​from resultsdb.models.results import Group, Result, Testcase, ResultData
41from resultsdb.models.results import JOB_STATUS, RESULT_OUTCOME 40​from resultsdb.models.results import JOB_STATUS, RESULT_OUTCOME
42 41​
42​# Only import fedmsg when publishing is turned on
43​if app.config['FEDMSG_PUBLISH']:
44​ import fedmsg
45​
43QUERY_LIMIT = 20 46​QUERY_LIMIT = 20
44 47​
45api = Blueprint('api_v1', __name__) 48​api = Blueprint('api_v1', __name__)
46 49​
47# TODO: find out why error handler works for 404 but not fot 400 50​# TODO: find out why error handler works for 404 but not fot 400
48 51​
49 52​
50@app.errorhandler(400) 53​@app.errorhandler(400)
▲ Show 20 Lines • Show All 679 Lines • Show Last 20 Lines
View Options

resultsdb/controllers/api_v2.py

Show All 25 Lines
26 26​
27from sqlalchemy.orm import exc as orm_exc 27​from sqlalchemy.orm import exc as orm_exc
28from werkzeug.exceptions import HTTPException 28​from werkzeug.exceptions import HTTPException
29 29​
30 30​
31from werkzeug.exceptions import BadRequest as JSONBadRequest 31​from werkzeug.exceptions import BadRequest as JSONBadRequest
32 32​
33import iso8601 33​import iso8601
34import fedmsg ​
35 34​
36from resultsdb import app, db 35​from resultsdb import app, db
37from resultsdb.serializers.api_v2 import Serializer 36​from resultsdb.serializers.api_v2 import Serializer
38from resultsdb.models.results import Group, Result, Testcase, ResultData 37​from resultsdb.models.results import Group, Result, Testcase, ResultData
39from resultsdb.models.results import RESULT_OUTCOME 38​from resultsdb.models.results import RESULT_OUTCOME
40 39​
40​# Only import fedmsg when publishing is turned on
41​if app.config['FEDMSG_PUBLISH']:
42​ import fedmsg
43​
41QUERY_LIMIT = 20 44​QUERY_LIMIT = 20
42 45​
43api = Blueprint('api_v2', __name__) 46​api = Blueprint('api_v2', __name__)
44 47​
45# TODO: find out why error handler works for 404 but not for 400 48​# TODO: find out why error handler works for 404 but not for 400
46 49​
47 50​
48@app.errorhandler(400) 51​@app.errorhandler(400)
▲ Show 20 Lines • Show All 683 Lines • Show Last 20 Lines