PR#314: New: About API - freshmaker

freshmaker

#314 New: About API

Merged 5 years ago by fivaldi. Opened 5 years ago by fivaldi.

fivaldi/freshmaker fivaldi_about_api into master

Filip Valder • 5 years ago

37df20f

freshmaker/__init__.py

file modified

		`@@ -23,6 +23,8 @@`
		`# Matt Prahl <mprahl@redhat.com>`
		`# Jan Kaluza <jkaluza@redhat.com>`

		`+ import pkg_resources`
		`+`
		`from logging import getLogger`

		`from flask import Flask`
		`@@ -33,6 +35,11 @@`
		`from freshmaker.config import init_config`
		`from freshmaker.proxy import ReverseProxy`

		`+ try:`
		`+ version = pkg_resources.get_distribution('freshmaker').version`
		`+ except pkg_resources.DistributionNotFound:`
		`+ version = 'unknown'`
		`+`
		`app = Flask(__name__)`
		`app.wsgi_app = ReverseProxy(app.wsgi_app)`

freshmaker/views.py

file modified

+25

		`@@ -30,6 +30,8 @@`
		`from freshmaker import models`
		`from freshmaker import types`
		`from freshmaker import db`
		`+ from freshmaker import conf`
		`+ from freshmaker import version`
		`from freshmaker.api_utils import filter_artifact_builds`
		`from freshmaker.api_utils import filter_events`
		`from freshmaker.api_utils import json_error`
		`@@ -121,6 +123,14 @@`
		`},`
		`},`
		`'monitor': MonitorAPI.rest_api_v1,`
		`+ 'about': {`
		`+ 'about': {`
		`+ 'url': '/api/1/about/',`
		`+ 'options': {`
		`+ 'methods': ['GET'],`
		`+ }`
		`+ },`
		`+ }`
		`}`


		`@@ -274,6 +284,20 @@`
		`return jsonify(db_event.json()), 200`


		`+ class AboutAPI(MethodView):`
		`+ def get(self):`
		`+ json = {'version': version}`
		`+ config_items = ['auth_backend']`
		`+ for item in config_items:`
		`+ config_item = getattr(conf, item)`
		`+ # All config items have a default, so if doesn't exist it is an error`
		`+ if not config_item:`
		`+ raise ValueError(`
		`+ 'An invalid config item of "{0}" was specified'.format(item))`
		`+ json[item] = config_item`
		`+ return jsonify(json), 200`
		`+`
		`+`
		`API_V1_MAPPING = {`
		`'events': EventAPI,`
		`'builds': BuildAPI,`
		`@@ -281,6 +305,7 @@`
		`'build_types': BuildTypeAPI,`
		`'build_states': BuildStateAPI,`
		`'monitor': MonitorAPI,`
		`+ 'about': AboutAPI,`
		`}`

tests/test_views.py

file modified

		`@@ -407,6 +407,12 @@`
		`self.assertEqual(data['error'], 'Not Found')`
		`self.assertEqual(data['message'], 'No such build state found.')`

		`+ def test_about_api(self):`
		`+ resp = self.client.get('/api/1/about/')`
		`+ data = json.loads(resp.get_data(as_text=True))`
		`+ # version is 'unknown' in case of skip_install=True in tox.ini`
		`+ self.assertEqual(data['version'], 'unknown')`
		`+`

		`class TestViewsMultipleFilterValues(helpers.ModelsTestCase):`
		`def setUp(self):`

no initial comment

fivaldi commented on line 6 of freshmaker/views.py 5 years ago

These 2 new imports could be merged with all the others before into a single import statement. Yes/no?

Edited 5 years ago by fivaldi

fivaldi commented on line 40 of freshmaker/views.py 5 years ago

Somebody has some ideas on some interesting config items to be exposed here?

lholecek commented on line 40 of freshmaker/views.py 5 years ago

Greenwave has only version but WaiverDB provides info on authentication method: https://waiverdb.engineering.redhat.com/api/v1.0/about

fivaldi commented on line 40 of freshmaker/views.py 5 years ago

Maybe auth_backend makes sense for freshmaker. This can be: noauth or openidc

rebased onto 4411513925cd6a9ff7ba2a792335978b63a40699

5 years ago

fivaldi commented on line 40 of freshmaker/views.py 5 years ago

@lholecek I put auth_backend here..

jkaluza commented 5 years ago

cqi commented 5 years ago

Code looks good for me. But I have a different thought about raising ProgrammingError from AboutAPI.get.

Let's see a case. An incorrect config is added to config_items, then freshmaker is released. When about API is called at some time after the release, client gets a bad request which represents the raised ProgrammingError. This shouldn't make sense. Instead, it would be good to catch the invalid config added to config_items during development. An assert could be appropriate for that.

Edited 5 years ago by cqi

rebased onto e4f0384bb2b6a2b61af90b6c16a8a8c730ca7a03

5 years ago

fivaldi commented 5 years ago

@cqi, thanks for pointing it out, I used ValueError instead.

rebased onto 37df20f

5 years ago