PR#665: Changes in MBS restful API - fm-orchestrator

fm-orchestrator

#665 Changes in MBS restful API

Merged 7 years ago by fivaldi. Opened 7 years ago by fivaldi.

fivaldi/fm-orchestrator fivaldi_component_api into master

Filip Valder • 7 years ago

3cb41aa

README.rst

file modified

+51 -9

		`@@ -79,10 +79,9 @@`
		`Client-side API`
		`===============`

		`- The orchestrator implements a RESTful interface for module build`
		`- submission and state querying. Not all REST methods are supported. See`
		`- below for details. For client tooling which utilizes the API, please`
		- refer to `Client tooling`_ section.
		`+ The MBS implements a RESTful interface for module build submission and state`
		`+ querying. Not all REST methods are supported. See below for details. For client`
		+ tooling which utilizes the API, please refer to `Client tooling`_ section.

		`Module build submission`
		`-----------------------`
		`@@ -158,11 +157,11 @@`
		`...`
		`}`

		`- "id" is the ID of the task. "state" refers to the orchestrator module`
		`- build state and might be one of "init", "wait", "build", "done", "failed" or`
		`- "ready". "tasks" is a dictionary of information about the individual component`
		`- builds including their IDs in the backend buildsystem, their state, a reason`
		`- for their state, and the NVR (if known).`
		`+ "id" is the ID of the task. "state" refers to the MBS module build state and`
		`+ might be one of "init", "wait", "build", "done", "failed" or "ready". "tasks"`
		`+ is a dictionary of information about the individual component builds including`
		`+ their IDs in the backend buildsystem, their state, a reason for their state,`
		`+ and the NVR (if known).`

		By adding ``?verbose=1`` to the request, additional detailed information
		`about the module can be obtained.`
		`@@ -409,6 +408,49 @@`
		`"total": 3`
		`}`

		`+ Component build state query`
		`+ ---------------------------`
		`+`
		`+ Getting particular component build is very similar to a module build query.`
		`+`
		`+ ::`
		`+`
		`+ GET /module-build-service/1/component-builds/1`
		`+`
		`+ The response, if the build exists, would include various pieces of information`
		`+ about the referenced component build.`
		`+`
		`+ ::`
		`+`
		`+ HTTP 200 OK`
		`+`
		`+ ::`
		`+`
		`+ {`
		`+ "format": "rpms",`
		`+ "id": 1,`
		`+ "module_build": 1,`
		`+ "package": "nginx",`
		`+ "state": 1,`
		`+ "state_name": "COMPLETE",`
		`+ "state_reason": null,`
		`+ "task_id": 12312345`
		`+ }`
		`+`
		`+ "id" is the ID of the component build. "state_name" refers to the MBS component`
		`+ build state and might be one of "COMPLETE", "FAILED", "CANCELED". "task_id"`
		`+ is a related task ID in the backend buildsystem, their state and a reason`
		`+ for their state. "module_build" refers to the module build ID for which this`
		`+ component was built. "format" is typically "rpms", since we're building it`
		`+ and "package" is simply the package name.`
		`+`
		+ By adding ``?verbose=1`` to the request, additional detailed information
		`+ about the component can be obtained.`
		`+`
		`+ ::`
		`+`
		`+ GET /module-build-service/1/component-builds/1?verbose=1`
		`+`
		`Listing component builds`
		`------------------------`

module_build_service/models.py

file modified

+52 -44

		`@@ -79,6 +79,19 @@`
		`INVERSE_BUILD_STATES = {v: k for k, v in BUILD_STATES.items()}`


		`+ def _utc_datetime_to_iso(datetime_object):`
		`+ """`
		`+ Takes a UTC datetime object and returns an ISO formatted string`
		`+ :param datetime_object: datetime.datetime`
		`+ :return: string with datetime in ISO format`
		`+ """`
		`+ if datetime_object:`
		`+ # Converts the datetime to ISO 8601`
		`+ return datetime_object.strftime("%Y-%m-%dT%H:%M:%SZ")`
		`+`
		`+ return None`
		`+`
		`+`
		`@contextlib.contextmanager`
		`def _dummy_context_mgr():`
		`"""`
		`@@ -290,8 +303,8 @@`
		`return []`

		`local_modules = [m for m in local_modules`
		`- if m.koji_tag`
		`- and m.koji_tag.startswith(conf.mock_resultsdir)]`
		`+ if m.koji_tag and`
		`+ m.koji_tag.startswith(conf.mock_resultsdir)]`
		`return local_modules`

		`@classmethod`
		`@@ -335,59 +348,37 @@`
		`def json(self):`
		`return {`
		`'id': self.id,`
		`- 'name': self.name,`
		`- 'stream': self.stream,`
		`- 'version': self.version,`
		`'state': self.state,`
		`'state_name': INVERSE_BUILD_STATES[self.state],`
		`'state_reason': self.state_reason,`
		`- 'state_url': get_url_for('module_build', id=self.id),`
		`- 'scmurl': self.scmurl,`
		`'owner': self.owner,`
		`- 'time_submitted': self._utc_datetime_to_iso(self.time_submitted),`
		`- 'time_modified': self._utc_datetime_to_iso(self.time_modified),`
		`- 'time_completed': self._utc_datetime_to_iso(self.time_completed),`
		`- "tasks": self.tasks(),`
		`+ 'name': self.name,`
		`+ 'scmurl': self.scmurl,`
		`+ 'time_submitted': _utc_datetime_to_iso(self.time_submitted),`
		`+ 'time_modified': _utc_datetime_to_iso(self.time_modified),`
		`+ 'time_completed': _utc_datetime_to_iso(self.time_completed),`
		`+ 'koji_tag': self.koji_tag,`
		`+ 'tasks': self.tasks(),`
		`+ }`
		`+`
		`+ def extended_json(self):`
		`+ json = self.json()`
		`+ json.update({`
		`+ 'stream': self.stream,`
		`+ 'version': self.version,`
		`+ 'state_url': get_url_for('module_build', id=self.id),`
		`# TODO, show their entire .json() ?`
		`'component_builds': [build.id for build in self.component_builds],`
		`'modulemd': self.modulemd,`
		`- 'koji_tag': self.koji_tag,`
		`- 'state_trace': [{'time': self._utc_datetime_to_iso(record.state_time),`
		`+ 'state_trace': [{'time': _utc_datetime_to_iso(record.state_time),`
		`'state': record.state,`
		`'state_name': INVERSE_BUILD_STATES[record.state],`
		`'reason': record.state_reason}`
		`for record`
		`in self.state_trace(self.id)]`
		`- }`
		`-`
		`- @staticmethod`
		`- def _utc_datetime_to_iso(datetime_object):`
		`- """`
		`- Takes a UTC datetime object and returns an ISO formatted string`
		`- :param datetime_object: datetime.datetime`
		`- :return: string with datetime in ISO format`
		`- """`
		`- if datetime_object:`
		`- # Converts the datetime to ISO 8601`
		`- return datetime_object.strftime("%Y-%m-%dT%H:%M:%SZ")`
		`-`
		`- return None`
		`+ })`

		`- def api_json(self):`
		`- return {`
		`- "id": self.id,`
		`- "state": self.state,`
		`- 'state_name': INVERSE_BUILD_STATES[self.state],`
		`- 'state_reason': self.state_reason,`
		`- "owner": self.owner,`
		`- "name": self.name,`
		`- "scmurl": self.scmurl,`
		`- "time_submitted": self._utc_datetime_to_iso(self.time_submitted),`
		`- "time_modified": self._utc_datetime_to_iso(self.time_modified),`
		`- "time_completed": self._utc_datetime_to_iso(self.time_completed),`
		`- "koji_tag": self.koji_tag,`
		`- "tasks": self.tasks()`
		`- }`
		`+ return json`

		`def tasks(self):`
		`"""`
		`@@ -435,7 +426,7 @@`
		`retval = {`
		`'id': self.id,`
		`'module_id': self.module_id,`
		`- 'state_time': self._utc_datetime_to_iso(self.state_time),`
		`+ 'state_time': _utc_datetime_to_iso(self.state_time),`
		`'state': self.state,`
		`'state_reason': self.state_reason,`
		`}`
		`@@ -495,6 +486,10 @@`
		`return session.query(cls).filter_by(`
		`package=component_name, module_id=module_id).first()`

		`+ def state_trace(self, component_id):`
		`+ return ComponentBuildTrace.query.filter_by(`
		`+ component_id=component_id).order_by(ComponentBuildTrace.state_time).all()`
		`+`
		`def json(self):`
		`retval = {`
		`'id': self.id,`
		`@@ -516,6 +511,19 @@`

		`return retval`

		`+ def extended_json(self):`
		`+ json = self.json()`
		`+ json.update({`
		`+ 'state_trace': [{'time': _utc_datetime_to_iso(record.state_time),`
		`+ 'state': record.state,`
		`+ 'state_name': INVERSE_BUILD_STATES[record.state],`
		`+ 'reason': record.state_reason}`
		`+ for record`
		`+ in self.state_trace(self.id)]`
		`+ })`
		`+`
		`+ return json`
		`+`
		`def __repr__(self):`
		`return "<ComponentBuild %s, %r, state: %r, task_id: %r, batch: %r, state_reason: %s>" % (`
		`self.package, self.module_id, self.state, self.task_id, self.batch, self.state_reason)`
		`@@ -536,7 +544,7 @@`
		`retval = {`
		`'id': self.id,`
		`'component_id': self.component_id,`
		`- 'state_time': self._utc_datetime_to_iso(self.state_time),`
		`+ 'state_time': _utc_datetime_to_iso(self.state_time),`
		`'state': self.state,`
		`'state_reason': self.state_reason,`
		`'task_id': self.task_id,`

module_build_service/scheduler/handlers/modules.py

file modified

+1 -1

		`@@ -150,7 +150,7 @@`
		`log.info("Found build=%r from message" % build)`
		`log.info("%r", build.modulemd)`

		`- module_info = build.json()`
		`+ module_info = build.extended_json()`
		`if module_info['state'] != msg.module_build_state:`
		`log.warn("Note that retrieved module state %r "`
		`"doesn't match message module state %r" % (`

module_build_service/views.py

file modified

+24 -16

		`@@ -67,6 +67,12 @@`
		`'methods': ['GET'],`
		`}`
		`},`
		`+ 'component_build': {`
		`+ 'url': '/module-build-service/1/component-builds/<int:id>',`
		`+ 'options': {`
		`+ 'methods': ['GET'],`
		`+ }`
		`+ },`
		`}`


		`@@ -84,23 +90,23 @@`
		`}`

		`if verbose_flag.lower() == 'true' or verbose_flag == '1':`
		`- json_data['items'] = [item.api_json() for item in p_query.items]`
		`+ json_data['items'] = [item.extended_json() for item in p_query.items]`
		`else:`
		`json_data['items'] = [{'id': item.id, 'state': item.state} for`
		`item in p_query.items]`

		`return jsonify(json_data), 200`
		`else:`
		`- # Lists details for the specified module builds`
		`- module = models.ComponentBuild.query.filter_by(id=id).first()`
		`+ # Lists details for the specified component builds`
		`+ component = models.ComponentBuild.query.filter_by(id=id).first()`

		`- if module:`
		`+ if component:`
		`if verbose_flag.lower() == 'true' or verbose_flag == '1':`
		`- return jsonify(module.json()), 200`
		`+ return jsonify(component.extended_json()), 200`
		`else:`
		`- return jsonify(module.api_json()), 200`
		`+ return jsonify(component.json()), 200`
		`else:`
		`- raise NotFound('No such module found.')`
		`+ raise NotFound('No such component found.')`


		`class ModuleBuildAPI(MethodView):`
		`@@ -117,7 +123,7 @@`
		`}`

		`if verbose_flag.lower() == 'true' or verbose_flag == '1':`
		`- json_data['items'] = [item.api_json() for item in p_query.items]`
		`+ json_data['items'] = [item.extended_json() for item in p_query.items]`
		`else:`
		`json_data['items'] = [{'id': item.id, 'state': item.state} for`
		`item in p_query.items]`
		`@@ -129,9 +135,9 @@`

		`if module:`
		`if verbose_flag.lower() == 'true' or verbose_flag == '1':`
		`- return jsonify(module.json()), 200`
		`+ return jsonify(module.extended_json()), 200`
		`else:`
		`- return jsonify(module.api_json()), 200`
		`+ return jsonify(module.json()), 200`
		`else:`
		`raise NotFound('No such module found.')`

		`@@ -150,7 +156,7 @@`

		`handler.validate()`
		`module = handler.post()`
		`- return jsonify(module.json()), 201`
		`+ return jsonify(module.extended_json()), 201`

		`def patch(self, id):`
		`username, groups = module_build_service.auth.get_user(request)`
		`@@ -194,7 +200,7 @@`
		`db.session.add(module)`
		`db.session.commit()`

		`- return jsonify(module.api_json()), 200`
		`+ return jsonify(module.json()), 200`


		`class BaseHandler(object):`
		`@@ -286,15 +292,17 @@`
		`module_view = ModuleBuildAPI.as_view('module_builds')`
		`component_view = ComponentBuildAPI.as_view('component_builds')`
		`for key, val in api_v1.items():`
		`- if key != 'component_builds_list':`
		`+ if key.startswith('component_build'):`
		`app.add_url_rule(val['url'],`
		`endpoint=key,`
		`- view_func=module_view,`
		`+ view_func=component_view,`
		`**val['options'])`
		`- else:`
		`+ elif key.startswith('module_build'):`
		`app.add_url_rule(val['url'],`
		`endpoint=key,`
		`- view_func=component_view,`
		`+ view_func=module_view,`
		`**val['options'])`
		`+ else:`
		`+ raise NotImplementedError("Unhandled api key.")`

		`register_api_v1()`

test-requirements.txt

file modified

		`@@ -1,5 +1,6 @@`
		`copr`
		`mock`
		`nose`
		`+ parameterized`
		`pytest`
		`vcrpy`

tests/test_scheduler/test_module_wait.py

file modified

+1 -1

		`@@ -66,7 +66,7 @@`
		`builder.module_build_tag = {'name': 'some-tag-build'}`
		`create_builder.return_value = builder`
		`mocked_module_build = mock.Mock()`
		`- mocked_module_build.json.return_value = {`
		`+ mocked_module_build.extended_json.return_value = {`
		`'name': 'foo',`
		`'stream': 1,`
		`'version': 1,`

tests/test_views/test_views.py

file modified

+43 -2

		`@@ -32,6 +32,7 @@`
		`from shutil import copyfile`
		`from os import path, mkdir`
		`from os.path import dirname`
		`+ from parameterized import parameterized`
		`import hashlib`

		`from tests import app, init_data`
		`@@ -256,12 +257,52 @@`
		`self.assertEquals(item['time_modified'], '2016-09-03T11:25:32Z')`
		`self.assertEquals(item['time_submitted'], '2016-09-03T11:23:20Z')`

		`- def test_query_builds_filter_nvr(self):`
		`+ def test_query_component_build(self):`
		`+ rv = self.client.get('/module-build-service/1/component-builds/1')`
		`+ data = json.loads(rv.data)`
		`+ self.assertEquals(data['id'], 1)`
		`+ self.assertEquals(data['format'], 'rpms')`
		`+ self.assertEquals(data['module_build'], 1)`
		`+ self.assertEquals(data['package'], 'nginx')`
		`+ self.assertEquals(data['state'], 1)`
		`+ self.assertEquals(data['state_name'], 'COMPLETE')`
		`+ self.assertEquals(data['state_reason'], None)`
		`+ self.assertEquals(data['task_id'], 12312345)`
		`+`
		`+ def test_query_component_build_verbose(self):`
		`+ rv = self.client.get('/module-build-service/1/component-builds/3?verbose=1')`
		`+ data = json.loads(rv.data)`
		`+ self.assertEquals(data['id'], 3)`
		`+ self.assertEquals(data['format'], 'rpms')`
		`+ self.assertEquals(data['module_build'], 2)`
		`+ self.assertEquals(data['package'], 'postgresql')`
		`+ self.assertEquals(data['state'], 1)`
		`+ self.assertEquals(data['state_name'], 'COMPLETE')`
		`+ self.assertEquals(data['state_reason'], None)`
		`+ self.assertEquals(data['task_id'], 2433433)`
		`+ self.assertEquals(data['state_trace'][0]['reason'], None)`
		`+ self.assertTrue(data['state_trace'][0]['time'] is not None)`
		`+ self.assertEquals(data['state_trace'][0]['state'], 1)`
		`+ self.assertEquals(data['state_trace'][0]['state_name'], 'wait')`
		`+`
		`+ component_builds_filters = ['tagged', 'ref', 'format']`
		`+`
		`+ @parameterized.expand([`
		`+ ('format', 'rpms', 60),`
		`+ ('ref', 'this-filter-query-should-return-zero-items', 0),`
		`+ ('tagged', 'this-filter-query-should-return-zero-items', 0),`
		`+ ])`
		`+ def test_query_component_builds_filters(self, f, s, c):`
		`+ rv = self.client.get('/module-build-service/1/component-builds/?{}={}'.format(f, s))`
		`+ data = json.loads(rv.data)`
		`+ self.assertEquals(data['meta']['total'], c)`
		`+`
		`+ def test_query_component_builds_filter_nvr(self):`
		`rv = self.client.get('/module-build-service/1/component-builds/?nvr=nginx-1.10.1-2.module_nginx_1_2')`
		`data = json.loads(rv.data)`
		`self.assertEquals(data['meta']['total'], 10)`

		`- def test_query_builds_filter_task_id(self):`
		`+ def test_query_component_builds_filter_task_id(self):`
		`rv = self.client.get('/module-build-service/1/component-builds/?task_id=12312346')`
		`data = json.loads(rv.data)`
		`self.assertEquals(data['meta']['total'], 1)`

tox.ini

file modified

		`@@ -14,6 +14,7 @@`
		`copr`
		`mock`
		`nose`
		`+ parameterized`
		`pytest`
		`vcrpy`
		`commands = py.test {posargs}`

fivaldi commented 7 years ago

Fixes #654

Changes in MBS restful API
- Refactor MBS API code
- Unify module-/component_build API philosophy/design/approach
- Naming fixes
- _utc_datetime_to_iso moved from ModuleBuildAPI and is now a module-level function.
- Existing v1 API remains unchanged. ComponentBuildAPI now supports individual component build listing + verbose mode.
- documented in README
- various component_build API tests added

mprahl commented on line 43 of module_build_service/models.py 7 years ago

For consistency, can you use single quotes or double quotes in the dictionary? I prefer single quotes, but consistency is what I'm most after.

ralph commented 7 years ago

:+1: to the gist.

Also, :+1: to @mprahl's comment.

Once the TODOs are knocked out let's merge this. :)

3 new commits added

final stuff ; to be squashed
tests
double quotes -> single quotes

7 years ago

fivaldi commented 7 years ago

PR just updated with tests, README updates and @mprahl 's recommendation. Please for second round of your review... TODO: Verify test results.

ralph commented 7 years ago

OK - I ran the tests and got a few failures. Here's a fix for one of them:

diff --git a/tests/test_views/test_views.py b/tests/test_views/test_views.py
index ab491e5..c579d30 100644
--- a/tests/test_views/test_views.py
+++ b/tests/test_views/test_views.py
@@ -257,7 +257,7 @@ class TestViews(unittest.TestCase):
         self.assertEquals(item['time_submitted'], '2016-09-03T11:23:20Z')

     def test_query_component_build(self):
-        rv = self.client.get('/module-build-service/1/component-build/1')
+        rv = self.client.get('/module-build-service/1/component-builds/1')
         data = json.loads(rv.data)
         self.assertEquals(data['id'], 1)
         self.assertEquals(data['format'], 'rpms')
@@ -269,7 +269,7 @@ class TestViews(unittest.TestCase):
         self.assertEquals(data['task_id'], 12312345)

     def test_query_component_build_verbose(self):
-        rv = self.client.get('/module-build-service/1/component-build/3?verbose=1')
+        rv = self.client.get('/module-build-service/1/component-builds/3?verbose=1')
         data = json.loads(rv.data)
         self.assertEquals(data['id'], 3)
         self.assertEquals(data['format'], 'rpms')

And then, I'd also make this addition to the api wiring code, just to be future-proof:

diff --git a/module_build_service/views.py b/module_build_service/views.py
index 6c45145..f557820 100644
--- a/module_build_service/views.py
+++ b/module_build_service/views.py
@@ -302,5 +302,7 @@ def register_api_v1():
                              endpoint=key,
                              view_func=module_view,
                              **val['options'])
+        else:
+            raise NotImplementedError("Unhandled api key.")

 register_api_v1()

After that, I still get one more test failure and I'm not sure why it's failing.

Can you run them and see if you can figure out why?

1 new commit added

@ralph's additions and his + other fixes for tests

7 years ago

rebased onto 3cb41aa

7 years ago

fivaldi commented 7 years ago

@ralph Thanks for those fixes :thumbsup:. I superseded the failing test with additional parametrized tests for all filters which should be supported in component_build API.

A.T.M. it should be ready for merge! :)

ralph commented 7 years ago

Cool. Looks good here. :)

Pull-Request has been merged by fivaldi

7 years ago