PR#2: Prototype of the POST /decision API - greenwave

greenwave

#2 Prototype of the POST /decision API

Merged 6 years ago by mjia. Opened 6 years ago by mjia.

mjia/greenwave master into master

prototype of the POST /decision API

Matt Jia • 6 years ago

3df0362

README.md

file modified

+46

		`@@ -4,3 +4,49 @@`
		`gating points in a software delivery pipeline, based on test results stored in`
		`[ResultsDB](https://pagure.io/taskotron/resultsdb) and waivers stored in`
		`[WaiverDB](https://pagure.io/waiverdb).`
		`+`
		`+ ## Quick development setup`
		`+`
		`+ Set up a python virtualenv:`
		`+`
		`+ $ sudo dnf install python-virtualenv`
		`+ $ virtualenv env_greenwave`
		`+ $ source env_greenwave/bin/activate`
		`+ $ pip install -r requirements.txt`
		`+ $ pip install -r dev-requirements.txt`
		`+`
		`+ Install the project:`
		`+`
		`+ $ python setup.py develop`
		`+`
		`+ Run the server:`
		`+`
		`+ $ python run-dev-server.py`
		`+`
		`+ The server is now running at <http://localhost:5005> and API calls can be sent to`
		`+ <http://localhost:5005/api/v1.0>.`
		`+`
		`+ ## Adjusting configuration`
		`+`
		+ You can configure this app by copying `conf/settings.py.example` into
		+ `conf/setting.py` and adjusting values as you see fit. It overrides default
		+ values in `greenwave/config.py`.
		`+`
		`+ ## Running test suite`
		`+`
		`+ You can run this test suite with the following command::`
		`+`
		`+ $ py.test greenwave/tests/`
		`+`
		`+ To test against all supported versions of Python, you can use tox::`
		`+`
		`+ $ sudo dnf install python3-tox`
		`+ $ tox`
		`+`
		`+ ## Building the docs`
		`+`
		`+ You can view the docs locally with::`
		`+`
		`+ $ cd docs`
		`+ $ make html`
		`+ $ firefox _build/html/index.html`

conf/settings.py.example

file added

		`@@ -0,0 +1,6 @@`
		+ # Copy this file to `conf/settings.py` to put it into effect. It overrides the values defined
		+ # in `greenwave/config.py`.
		`+ SECRET_KEY = 'replace-me-with-something-random'`
		`+ JOURNAL_LOGGING = False`
		`+ HOST= '0.0.0.0'`
		`+ PORT = 5005`

dev-requirements.txt

file modified

		`@@ -3,6 +3,7 @@`
		`mock`
		`pytest`
		`pytest-cov`
		`+ requests_mock`

		`# Documentation build requirements`
		`sphinx`

greenwave/api_v1.py

file added

+121

		`@@ -0,0 +1,121 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+`
		`+ import requests`
		`+ from flask import Blueprint, request, current_app, jsonify`
		`+ from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType`
		`+ from greenwave.policies import policies`
		`+`
		`+ api = (Blueprint('api_v1', __name__))`
		`+`
		`+`
		`+ @api.route('/decision', methods=['POST'])`
		`+ def make_decision():`
jcline commented 6 years ago I suggest refactoring this function into several smaller functions that the route handler ties together. It's difficult to follow due to the multiple nested loops and if statements.
mjia commented 6 years ago Yeah, I believe this will more likely be changed when more policies are added. I would like to keep it like that for the first version.
		`+ """`
		`+ Make a decision after evaluating all applicable policies based on test`
		`+ results. The request must be`
		+ :mimetype:`application/json`.
		`+`
		`+ :jsonparam string product_version: The product version string used for querying WaiverDB.`
		`+ :jsonparam string decision_context: The decision context string.`
		`+ :jsonparam array subject: A list of items about which the caller is requesting a decision`
		`+ used for querying ResultsDB. For example, a list of build NVRs.`
		`+ :statuscode 200: A decision was made.`
		`+ :statuscode 400: Invalid data was given.`
		`+ """`
		`+ if request.get_json():`
jcline commented 6 years ago I know there was a decision to not use a REST framework, but this schema validation could be handled in a much more readable way with any number of libraries. Something like marshmallow (which, incidentally, flask-restful is going to start recommending) would do this and it pulls the schema definition/validation out of the request handler. Just a thought.
mjia commented 6 years ago Marshmallow might be a good option if we have many APIs doing this like that. For this small app, it might be overkill. I would rather improve the error messages to be more readable.
		`+ if ('product_version' not in request.get_json() or`
		`+ not request.get_json()['product_version']):`
		`+ raise BadRequest('Missing required product version')`
		`+ if ('decision_context' not in request.get_json() or`
		`+ not request.get_json()['decision_context']):`
		`+ raise BadRequest('Missing required decision context')`
		`+ if ('subject' not in request.get_json() or`
		`+ not request.get_json()['subject']):`
		`+ raise BadRequest('Missing required subject')`
		`+ else:`
		`+ raise UnsupportedMediaType('No JSON payload in request')`
		`+ if not isinstance(request.get_json()['subject'], list):`
		`+ raise BadRequest('Invalid subject, must be a list of items')`
		`+ product_version = request.get_json()['product_version']`
		`+ decision_context = request.get_json()['decision_context']`
		`+ applicable_policies = {}`
		`+ for policy_id, policy in policies.iteritems():`
		`+ if product_version == policy['product_version'] and \`
		`+ decision_context == policy['decision_context']:`
		`+ applicable_policies[policy_id] = policy`
		`+ if not applicable_policies:`
		`+ raise NotFound('Cannot find any applicable policies for %s' % product_version)`
		`+ subjects = [item.strip() for item in request.get_json()['subject'] if item]`
		`+ policies_satisified = True`
		`+ summary = []`
		`+ unsatisfied_requirements = []`
		`+ with requests.Session() as s:`
jcline commented 6 years ago This approach defeats the purpose of using a session since the session is discarded after each request is handled. The session must live beyond the request/response cycle.
mjia commented 6 years ago I'm not quite sure what you mean since I'm using a context manager here which creates a global session. It will be closed once all the requests inside the context manager are handled.
		`+ for policy_id, policy in applicable_policies.iteritems():`
		`+ for item in subjects:`
		`+ res = s.get('{0}/results?item={1}&testcases={2}'.format(`
		`+ current_app.config['RESULTSDB_API_URL'], item, ','.join(policy['rules']))`
		`+ )`
		`+ res.raise_for_status()`
jcline commented 6 years ago I don't see any error handling code for the exception this raises, so the response is likely an HTTP 500 rather than a more useful error message about the service being unavailable or something. The cleanest way to handle this likely with an error handler for the various requests exception types.
mjia commented 6 years ago Yeah, I will fix this in the following PRs.
		`+ results = res.json()['data']`
		`+ total_failed_results = 0`
		`+ if results:`
		`+ for result in results:`
		`+ if result['outcome'] not in ('PASSED', 'INFO'):`
		`+ # query WaiverDB to check whether the result has a waiver`
		`+ res = s.get('{0}/waivers/?project_version={1}&result_id={2}'.format(`
		`+ current_app.config['WAIVERDB_API_URL'], product_version,`
		`+ result['id'])`
		`+ )`
		`+ res.raise_for_status()`
		`+ waiver = res.json()['data']`
		`+ if not waiver or not waiver[0]['waived']:`
		`+ policies_satisified = False`
		`+ total_failed_results += 1`
		`+ unsatisfied_requirements.append({`
		`+ 'type': 'test-result-failed',`
		`+ 'item': item,`
		`+ 'testcase': result['testcase']['name'],`
		`+ 'result_id': result['id']})`
		`+ # find missing results`
		`+ rules_applied = [result['testcase']['name'] for result in results]`
		`+ for rule in policy['rules']:`
		`+ if rule not in rules_applied:`
		`+ total_failed_results += 1`
		`+ unsatisfied_requirements.append({`
		`+ 'type': 'test-result-missing',`
		`+ 'item': item,`
		`+ 'testcase': rule})`
		`+ if total_failed_results:`
		`+ summary.append(`
		`+ '{0}: {1} of {2} required tests failed, the policy {3} is not satisfied'`
		`+ .format(item, total_failed_results, len(policy['rules']),`
		`+ policy_id))`
		`+ else:`
		`+ summary.append(`
		`+ '%s: policy %s is satisfied as all required tests are passing' % (`
		`+ item, policy_id))`
		`+ else:`
		`+ policies_satisified = False`
		`+ summary.append('%s: no test results found' % item)`
		`+ for rule in policy['rules']:`
		`+ unsatisfied_requirements.append({`
		`+ 'type': 'test-result-missing',`
		`+ 'item': item,`
		`+ 'testcase': rule})`
		`+ res = {`
		`+ 'policies_satisified': policies_satisified,`
		`+ 'summary': '\n'.join(summary),`
		`+ 'applicable_policies': list(applicable_policies.keys()),`
		`+ 'unsatisfied_requirements': unsatisfied_requirements`
		`+ }`
		`+ return jsonify(res), 200`

greenwave/app_factory.py

file added

+48

		`@@ -0,0 +1,48 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+`
		`+ import os`
		`+`
		`+ from flask import Flask`
		`+ from greenwave.logger import init_logging`
		`+ from greenwave.api_v1 import api`
		`+`
		`+`
		`+ def load_config(app):`
		`+ # Load default config, then override that with a config file`
		`+ if os.getenv('DEV') == 'true':`
		`+ default_config_obj = 'greenwave.config.DevelopmentConfig'`
		`+ default_config_file = os.getcwd() + '/conf/settings.py'`
		`+ elif os.getenv('TEST') == 'true':`
		`+ default_config_obj = 'greenwave.config.TestingConfig'`
		`+ default_config_file = os.getcwd() + '/conf/settings.py'`
		`+ else:`
		`+ default_config_obj = 'greenwave.config.ProductionConfig'`
		`+ default_config_file = '/etc/greenwave/settings.py'`
		`+ app.config.from_object(default_config_obj)`
		`+ config_file = os.environ.get('GREENWAVE_CONFIG', default_config_file)`
		`+ app.config.from_pyfile(config_file)`
		`+`
		`+`
		`+ # applicaiton factory http://flask.pocoo.org/docs/0.12/patterns/appfactories/`
		`+ def create_app(config_obj=None):`
		`+ app = Flask(__name__)`
		`+ if config_obj:`
		`+ app.config.from_object(config_obj)`
		`+ else:`
		`+ load_config(app)`
		`+ if app.config['PRODUCTION'] and app.secret_key == 'replace-me-with-something-random':`
		`+ raise Warning("You need to change the app.secret_key value for production")`
		`+ # initialize logging`
		`+ init_logging(app)`
		`+ # register blueprints`
		`+ app.register_blueprint(api, url_prefix="/api/v1.0")`
		`+ return app`

greenwave/config.py

file added

+39

		`@@ -0,0 +1,39 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+`
		`+`
		`+ class Config(object):`
		`+ """`
		`+ A GreenWave Flask configuration.`
		`+ """`
		`+ DEBUG = True`
		`+ JOURNAL_LOGGING = False`
		`+ HOST = '0.0.0.0'`
		`+ PORT = 5005`
		`+ PRODUCTION = False`
		`+ SECRET_KEY = 'replace-me-with-something-random'`
		`+ RESULTSDB_API_URL = 'https://taskotron.fedoraproject.org/resultsdb_api/api/v2.0'`
		`+ WAIVERDB_API_URL = 'https://waiverdb.fedoraproject.org/api/v1.0'`
		`+`
		`+`
		`+ class ProductionConfig(Config):`
		`+ DEBUG = False`
		`+ PRODUCTION = True`
		`+`
		`+`
		`+ class DevelopmentConfig(Config):`
		`+ RESULTSDB_API_URL = 'https://taskotron.stg.fedoraproject.org/resultsdb_api/api/v2.0'`
		`+ WAIVERDB_API_URL = 'http://waiverdb-dev.fedorainfracloud.org/api/v1.0'`
		`+`
		`+`
		`+ class TestingConfig(Config):`
		`+ RESULTSDB_API_URL = 'https://resultsdb.domain.local/api/v2.0'`
		`+ WAIVERDB_API_URL = 'https://waiverdb.domain.local/api/v1.0'`

greenwave/logger.py

file added

+38

		`@@ -0,0 +1,38 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+`
		`+ import logging`
		`+ import sys`
		`+ import systemd.journal`
		`+`
		`+`
		`+ def log_to_stdout(app, level=logging.INFO):`
		`+ fmt = '[%(filename)s:%(lineno)d] ' if app.debug else '%(module)-12s '`
		`+ fmt += '%(asctime)s %(levelname)-7s %(message)s'`
		`+ datefmt = '%Y-%m-%d %H:%M:%S'`
		`+ stream_handler = logging.StreamHandler(sys.stdout)`
		`+ stream_handler.setLevel(level)`
		`+ stream_handler.setFormatter(logging.Formatter(fmt=fmt, datefmt=datefmt))`
		`+ app.logger.addHandler(stream_handler)`
		`+`
		`+`
		`+ def log_to_journal(app, level=logging.INFO):`
dcallagh commented 6 years ago If we are going full container, then we can probably take this stuff back out. There is no real solution for structured logging in OpenShift, it's all just plain text to stdout (a big step backwards IMHO).
dcallagh commented 6 years ago If we are going full container, then we can probably take this stuff back out. There is no real solution for structured logging in OpenShift, it's all just plain text to stdout (a big step backwards IMHO).
mjia commented 6 years ago Well, I would say it doesn't hurt to have this here. Edited 6 years ago by mjia
		`+ journal_handler = systemd.journal.JournalHandler()`
		`+ journal_handler.setLevel(level)`
		`+ app.logger.addHandler(journal_handler)`
		`+`
		`+`
		`+ def init_logging(app):`
		`+ log_level = logging.DEBUG if app.debug else logging.INFO`
		`+ if app.config['JOURNAL_LOGGING']:`
		`+ log_to_journal(app, level=log_level)`
		`+ else:`
		`+ log_to_stdout(app, level=log_level)`

greenwave/policies.py

file added

+28

		`@@ -0,0 +1,28 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+`
		`+ policies = {`
		`+ # Mimic the default Errata rule used for RHEL-7 https://errata.devel.redhat.com/workflow_rules/1`
		`+ # In Errata, in order to transition to QE state, an advisory must complete rpmdiff test.`
		`+ # A completed rpmdiff test could be some dist.rpmdiff.* testcases in ResultsDB and all the`
		`+ # tests need to be passed.`
dcallagh commented 6 years ago Actually looking at the ruleset it seems there is no actual requirement that the RPMDiff results pass or are waived. :-) But we can fix that in a later patch once this is merged...
		`+ '1': {`
		`+ 'product_version': 'rhel-7',`
		`+ 'decision_context': 'errta_newfile_to_qe',`
		`+ 'rules': [`
		`+ 'dist.rpmdiff.comparison.xml_validity',`
		`+ 'dist.rpmdiff.comparison.virus_scan',`
		`+ 'dist.rpmdiff.comparison.upstream_source',`
		`+ 'dist.rpmdiff.comparison.symlinks',`
		`+ 'dist.rpmdiff.comparison.binary_stripping']`
		`+ }`
		`+ }`

greenwave/tests/conftest.py

file added

+37

		`@@ -0,0 +1,37 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+`
		`+ import pytest`
		`+ from greenwave.app_factory import create_app`
		`+`
		`+`
		`+ @pytest.fixture(scope='session')`
		`+ def app(request):`
		`+ app = create_app('greenwave.config.TestingConfig')`
		`+ # Establish an application context before running the tests.`
		`+ ctx = app.app_context()`
		`+ ctx.push()`
		`+`
		`+ def teardown():`
		`+ ctx.pop()`
		`+`
		`+ request.addfinalizer(teardown)`
		`+ return app`
		`+`
		`+`
		`+ @pytest.yield_fixture`
		`+ def client(app):`
		+ """A Flask test client. An instance of :class:`flask.testing.TestClient`
		`+ by default.`
		`+ """`
		`+ with app.test_client() as client:`
		`+ yield client`

greenwave/tests/test_api_v10.py

file added

+301

		`@@ -0,0 +1,301 @@`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+`
		`+ import json`
		`+ import requests_mock`
		`+ from flask import current_app`
		`+ from mock import patch`
		`+`
		`+`
		`+ def test_cannot_make_decision_without_product_version(client):`
		`+ data = {`
		`+ 'decision_context': 'errta_newfile_to_qe',`
		`+ 'subject': ['foo-1.0.0-1.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 400`
		`+ assert 'Missing required product version' in r.get_data()`
		`+`
		`+`
		`+ def test_cannot_make_decision_without_decision_context(client):`
		`+ data = {`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-1.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 400`
		`+ assert 'Missing required decision context' in r.get_data()`
		`+`
		`+`
		`+ def test_cannot_make_decision_without_subject(client):`
		`+ data = {`
		`+ 'decision_context': 'errta_newfile_to_qe',`
		`+ 'product_version': 'rhel-7',`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 400`
		`+ assert 'Missing required subject' in r.get_data()`
		`+`
		`+`
		`+ def test_404_for_inapplicable_policies(client):`
		`+ data = {`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-1.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 404`
		`+ assert 'Cannot find any applicable policies for %s' % data['product_version'] in r.get_data()`
		`+`
		`+`
		`+ def test_make_a_decison_on_passed_result(client):`
		`+ with requests_mock.Mocker() as m:`
		`+ mocked_results = {`
		`+ "data": [`
		`+ {`
		`+ "data": {`
		`+ "item": [`
		`+ "foo-1.0.0-2.el7"`
		`+ ]`
		`+ },`
		`+ "groups": [`
		`+ "5d307e4f-1ade-4c41-9e67-e5a73d5cdd07"`
		`+ ],`
		`+ "href": "https://resultsdb.domain.local/api/v2.0/results/331284",`
		`+ "id": 331284,`
		`+ "note": "",`
		`+ "outcome": "PASSED",`
		`+ "ref_url": "https://rpmdiff.domain.local/run/97683/26",`
		`+ "submit_time": "2017-05-19T04:41:13.957729",`
		`+ "testcase": {`
		`+ "href": 'https://resultsdb.domain.local/api/v2.0/testcases/'`
		`+ 'dist.rpmdiff.comparison.xml_validity',`
		`+ "name": "dist.rpmdiff.comparison.xml_validity",`
		`+ "ref_url": "https://docs.domain.local/display/HTD/rpmdiff-valid-file"`
		`+ }`
		`+ }`
		`+ ]`
		`+ }`
		`+ m.register_uri('GET', '{}/results?item={}&testcases={}'.format(`
		`+ current_app.config['RESULTSDB_API_URL'],`
		`+ 'foo-1.0.0-2.el7',`
		`+ 'dist.rpmdiff.comparison.xml_validity'`
		`+ ), json=mocked_results)`
		`+ dummy_policies = {`
		`+ '1': {`
		`+ 'product_version': 'rhel-7',`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'rules': ['dist.rpmdiff.comparison.xml_validity']`
		`+ }`
		`+ }`
		`+ with patch.dict('greenwave.policies.policies', dummy_policies):`
		`+ data = {`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-2.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 200`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert res_data['policies_satisified'] == True`
		`+ assert res_data['applicable_policies'] == ['1']`
		`+ assert res_data['summary'] == 'foo-1.0.0-2.el7: policy 1 is satisfied as all required' \`
		`+ ' tests are passing'`
		`+`
		`+`
		`+ def test_make_a_decison_on_failed_result_with_waiver(client):`
		`+ with requests_mock.Mocker() as m:`
		`+ mocked_results = {`
		`+ "data": [`
		`+ {`
		`+ "data": {`
		`+ "item": [`
		`+ "foo-1.0.0-2.el7"`
		`+ ]`
		`+ },`
		`+ "groups": [`
		`+ "5d307e4f-1ade-4c41-9e67-e5a73d5cdd07"`
		`+ ],`
		`+ "href": "https://resultsdb.domain.local/api/v2.0/results/331284",`
		`+ "id": 331284,`
		`+ "note": "",`
		`+ "outcome": "FAILED",`
		`+ "ref_url": "https://rpmdiff.domain.local/run/97683/26",`
		`+ "submit_time": "2017-05-19T04:41:13.957729",`
		`+ "testcase": {`
		`+ "href": 'https://resultsdb.domain.local/api/v2.0/testcases/'`
		`+ 'dist.rpmdiff.comparison.xml_validity',`
		`+ "name": "dist.rpmdiff.comparison.xml_validity",`
		`+ "ref_url": "https://docs.domain.local/display/HTD/rpmdiff-valid-file"`
		`+ }`
		`+ }`
		`+ ]`
		`+ }`
		`+ m.register_uri('GET', '{}/results?item={}&testcases={}'.format(`
		`+ current_app.config['RESULTSDB_API_URL'],`
		`+ 'foo-1.0.0-2.el7',`
		`+ 'dist.rpmdiff.comparison.xml_validity'`
		`+ ), json=mocked_results)`
		`+ mocked_waiver = {`
		`+ "data": [`
		`+ {`
		`+ "id": 1,`
		`+ "result_id": 331284,`
		`+ "username": 'fool',`
		`+ "comment": 'it broke',`
		`+ "waived": True,`
		`+ "timestamp": '2017-05-17T03:13:31.735858',`
		`+ "product_version": 'rhel-7'`
		`+ }`
		`+ ]`
		`+ }`
		`+ m.register_uri('GET', '{}/waivers/?result_id={}&project_version={}'.format(`
		`+ current_app.config['WAIVERDB_API_URL'],`
		`+ 331284,`
		`+ 'rhel-7'`
		`+ ), json=mocked_waiver)`
		`+ dummy_policies = {`
		`+ '1': {`
		`+ 'product_version': 'rhel-7',`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'rules': ['dist.rpmdiff.comparison.xml_validity']`
		`+ }`
		`+ }`
		`+ with patch.dict('greenwave.policies.policies', dummy_policies):`
		`+ data = {`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-2.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 200`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert res_data['policies_satisified'] == True`
		`+ assert res_data['applicable_policies'] == ['1']`
		`+ assert res_data['summary'] == 'foo-1.0.0-2.el7: policy 1 is satisfied as all required' \`
		`+ ' tests are passing'`
		`+`
		`+`
		`+ def test_make_a_decison_on_failed_result(client):`
		`+ with requests_mock.Mocker() as m:`
		`+ mocked_results = {`
		`+ "data": [`
		`+ {`
		`+ "data": {`
		`+ "item": [`
		`+ "foo-1.0.0-2.el7"`
		`+ ]`
		`+ },`
		`+ "groups": [`
		`+ "5d307e4f-1ade-4c41-9e67-e5a73d5cdd07"`
		`+ ],`
		`+ "href": "https://resultsdb.domain.local/api/v2.0/results/331284",`
		`+ "id": 331284,`
		`+ "note": "",`
		`+ "outcome": "FAILED",`
		`+ "ref_url": "https://rpmdiff.domain.local/run/97683/26",`
		`+ "submit_time": "2017-05-19T04:41:13.957729",`
		`+ "testcase": {`
		`+ "href": 'https://resultsdb.domain.local/api/v2.0/testcases/'`
		`+ 'dist.rpmdiff.comparison.xml_validity',`
		`+ "name": "dist.rpmdiff.comparison.xml_validity",`
		`+ "ref_url": "https://docs.domain.local/display/HTD/rpmdiff-valid-file"`
		`+ }`
		`+ }`
		`+ ]`
		`+ }`
		`+ m.register_uri('GET', '{}/results?item={}&testcases={}'.format(`
		`+ current_app.config['RESULTSDB_API_URL'],`
		`+ 'foo-1.0.0-2.el7',`
		`+ 'dist.rpmdiff.comparison.xml_validity,dist.rpmdiff.comparison.virus_scan'`
		`+ ), json=mocked_results)`
		`+ m.register_uri('GET', '{}/waivers/?result_id={}&project_version={}'.format(`
		`+ current_app.config['WAIVERDB_API_URL'],`
		`+ 331284,`
		`+ 'rhel-7'`
		`+ ), json={"data": []})`
		`+ dummy_policies = {`
		`+ '1': {`
		`+ 'product_version': 'rhel-7',`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'rules': ['dist.rpmdiff.comparison.xml_validity',`
		`+ 'dist.rpmdiff.comparison.virus_scan']`
		`+ }`
		`+ }`
		`+ with patch.dict('greenwave.policies.policies', dummy_policies):`
		`+ data = {`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-2.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 200`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert res_data['policies_satisified'] == False`
		`+ assert res_data['applicable_policies'] == ['1']`
		`+ assert res_data['summary'] == 'foo-1.0.0-2.el7: 2 of 2 required tests' \`
		`+ ' failed, the policy 1 is not satisfied'`
		`+ expected_unsatisfied_requirements = [`
		`+ {`
		`+ 'item': 'foo-1.0.0-2.el7',`
		`+ 'result_id': 331284,`
		`+ 'testcase': 'dist.rpmdiff.comparison.xml_validity',`
		`+ 'type': 'test-result-failed'`
		`+ },`
		`+ {`
		`+ 'item': 'foo-1.0.0-2.el7',`
		`+ 'testcase': 'dist.rpmdiff.comparison.virus_scan',`
		`+ 'type': 'test-result-missing'`
		`+ }]`
		`+ assert res_data['unsatisfied_requirements'] == expected_unsatisfied_requirements`
		`+`
		`+`
		`+ def test_make_a_decison_on_no_results(client):`
		`+ with requests_mock.Mocker() as m:`
		`+ m.register_uri('GET', '{}/results?item={}&testcases={}'.format(`
		`+ current_app.config['RESULTSDB_API_URL'],`
		`+ 'foo-1.0.0-2.el7',`
		`+ 'dist.rpmdiff.comparison.xml_validity'`
		`+ ), json={"data": []})`
		`+ dummy_policies = {`
		`+ '1': {`
		`+ 'product_version': 'rhel-7',`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'rules': ['dist.rpmdiff.comparison.xml_validity']`
		`+ }`
		`+ }`
		`+ with patch.dict('greenwave.policies.policies', dummy_policies):`
		`+ data = {`
		`+ 'decision_context': 'dummpy_decision',`
		`+ 'product_version': 'rhel-7',`
		`+ 'subject': ['foo-1.0.0-2.el7']`
		`+ }`
		`+ r = client.post('/api/v1.0/decision', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ assert r.status_code == 200`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert res_data['policies_satisified'] == False`
		`+ assert res_data['applicable_policies'] == ['1']`
		`+ assert res_data['summary'] == 'foo-1.0.0-2.el7: no test results found'`
		`+ expected_unsatisfied_requirements = [`
		`+ {`
		`+ 'item': 'foo-1.0.0-2.el7',`
		`+ 'testcase': 'dist.rpmdiff.comparison.xml_validity',`
		`+ 'type': 'test-result-missing'`
		`+ }]`
		`+ assert res_data['unsatisfied_requirements'] == expected_unsatisfied_requirements`

requirements.txt

file modified

+2 -1

		`@@ -1,2 +1,3 @@`
		`flask`
		`- sqlalchemy`
		`+ systemd`
		`+ requests`

run-dev-server.py

file added

+22

		`@@ -0,0 +1,22 @@`
		`+ #!/usr/bin/python`
		`+`
		`+ # This program is free software; you can redistribute it and/or modify`
		`+ # it under the terms of the GNU General Public License as published by`
		`+ # the Free Software Foundation; either version 2 of the License, or`
		`+ # (at your option) any later version.`
		`+ #`
		`+ # This program is distributed in the hope that it will be useful,`
		`+ # but WITHOUT ANY WARRANTY; without even the implied warranty of`
		`+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the`
		`+ # GNU General Public License for more details.`
		`+ #`
		`+`
		`+ from greenwave.app_factory import create_app`
		`+`
		`+ if __name__ == '__main__':`
		`+ app = create_app('greenwave.config.DevelopmentConfig')`
		`+ app.run(`
		`+ host=app.config['HOST'],`
		`+ port=app.config['PORT'],`
		`+ debug=app.config['DEBUG'],`
		`+ )`

mjia commented 6 years ago

Imagine that Errata wants to move a RHEL-7 advisory from New Files to QE. It will send a query to Greenwave to ask for a decision.

A POST request could be sent to https://greenwave.example.com/api/v1.0/decision with a json body:
{
'product_version': 'rhel-7',
'decision_context' : 'errta_newfile_to_qe',
'subject' : ['java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3']
}

Greenwave returns a decision as a JSON object. If all the policies are satisfied, the response could be
{
'policies_satisified': True,
'summary': 'java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3: policy 1 is satisfied as all required tests are passing',
'applicable_policies': [1],
'unsatisfied_requirements': None
}
If one of the polices is not satisfied, the response could be
{
'policies_satisified': False,
'summary': 'java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3: 2 of 3 required tests failed, the policy 1 is not satisfied',
'applicable_policies': [1],
'unsatisfied_requirements': [
{
'item': 'java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3',
'testcase': 'dist.rpmdiff.comparison.xml_validity',
'type': 'test-result-missing'
} ,
{
'item': 'java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3',
'testcase': 'dist.rpmdiff.comparison.virus_scan',
'type': 'test-result-failed'
} ,
....
]
}
TODO: add more policies

jcline commented on line 18 of greenwave/api_v1.py 6 years ago

This is a duplicate import from above

jcline commented on line 31 of greenwave/api_v1.py 6 years ago

It might make more sense to have the request parsers live on the Resource classes

jcline commented on line 72 of greenwave/api_v1.py 6 years ago

We definitely want to use connection pooling here as well as a (possibly configurable) timeout on requests since by default they have no timeout. All you need to do is create a global Session for connection pooling.

jcline commented on line 80 of greenwave/api_v1.py 6 years ago

There's a lot of reasons that this could have not been an HTTP 200. For example, the service might be temporarily unavailable (HTTP 503). It seems odd to use HTTP 400 here, as well, since the request could be well-formed, but the resource simply doesn't exist (404).

jcline commented on line 121 of greenwave/api_v1.py 6 years ago

HTTP 201 seems incorrect. It's a GET request with the requested resource, so it should return HTTP 200.

jcline commented 6 years ago

I think we might want to think a bit about the URL scheme before I get too far into a review (and I know this is a WIP :grinning: )

GET /api/v1.0/decision?product_version=rhel-7&decision_context=errta_newfile_to_qe&subject=java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3

seems a odd to me. Since all three query parameters are required, they really should just be part of the URL, so it would look something like:

GET /api/v1.0/decision/<product_version>/<decision_context>/<subject>/

although maybe that order doesn't make logical sense.

ralph commented 6 years ago

Either option seems fine to me.

Pointing out that all three query parameters are required and that the request will return an error if any of them are missing is a valid observation. But the same problem maps to the alternative API you suggested, @jcline:

Since /decision/ is meaningless without all three, what do we do about half-formed URLs? Should GET /api/v1.0/decision/<product_version>/ return a 400 error or a 404 error?

That said, I do think I like the slash-delimited form a bit better.

jcline commented 6 years ago

I would expect such a request to return a 404. It's a valid URL and the request itself is well-formed, but there's nothing there. I know everyone has different opinions about which error response code to use, but I see HTTP 400 as being used for problems with the syntax of the request itself. e.g. a client tells me the Content-Length is a negative number, or that the body is application/json and it's not (or I have a schema for the JSON I accept and the request doesn't adhere to that schema).

They're basically equivalent, but the query string variety is just unusual (to me) as far as APIs go.

dcallagh commented 6 years ago

The three query params are required for now but we will probably want to relax that in future (or more likely, add more possible params). In particular, I am sure we will want to change the subject parameter in future. The RPMDiff results (where the item is a pair of RPMs) and the OpenQA results (where the item is a compose... I think) are going to make it more complicated.

In fact we will probably end up needing to make the endpoint accept POST with JSON so that we can express more complex values for the subject.

If the query params approach is not sitting right with people, we could just switch it to a POST with JSON now?

Either way I think putting the params into the URL path is not a good choice.

dcallagh commented 6 years ago

In fact we will probably end up needing to make the endpoint accept POST with JSON so that we can express more complex values for the subject.

If the query params approach is not sitting right with people, we could just switch it to a POST with JSON now?

Either way I think putting the params into the URL path is not a good choice.

dcallagh commented 6 years ago

(Sigh... how do I manage to keep double-commenting in Pagure?)

And yeah, the error code for a missing query param (or a query parameter value which doesn't match anything) should be 404.

404 means the URL doesn't exist, and the query string is a part of the URL just the same as the path is.

/decision?product_version=rhel-7&... -> 200
/decision?product_version=rehl-7&... -> 404 because the product doesn't exist
/decision -> 404 because a decision without any product doesn't exist

Of course the 404 would still ideally say what exactly is missing (not just "URL not found").

dcallagh commented on line 23 of greenwave/api_v1.py 6 years ago

Don't save these globally at import time -- the config will not be loaded yet. You will want to reference .config[...] inside each request handler as you need it.

dcallagh commented on line 33 of greenwave/api_v1.py 6 years ago

Flask-restful might be overkill for this service. You might find it is simpler to just write the request handlers by hand and grab the args out. It's up to you. I suggest try writing a version without flask-restful and if it is the same amount of code or less, then drop it. :-)

dcallagh commented on line 59 of greenwave/api_v1.py 6 years ago

If we don't find any applicable policies, we should probably assume the product is not a real product and give back a 404 indicating that.

dcallagh commented on line 80 of greenwave/api_v1.py 6 years ago

Yeah if we have an error responding to this request, it's not a BadRequest, it's a 5xx code.

For now the easiest approach is don't handle any errors at all, let everything raise out and give back a 500. We can refine the error handling later.

So for this code block that means you can just do:

response = requests.get(...)
response.raise_for_status()
results = response.json()['data']

Btw get_req is not a good variable name. The return type of requests.get() is a Response not a Request.

dcallagh commented on line 86 of greenwave/api_v1.py 6 years ago

We will probably want to factor this out into some kind of "matchers", then each policy would be composed of multiple matchers. Anyway this is fine for now. As we expand the policies (and once we have good tests!) we can refactor.

dcallagh commented on line 116 of greenwave/api_v1.py 6 years ago

Yeah so I think the "matchers" idea will help us here, with building up this output. We probably want this to be an array, where each item describes a single thing that was missing (or failed).

[ { 'type': 'test-result-missing', 'item': '...', 'testcase': '...' },
  { 'type': 'test-result-failed', 'item': '...', 'testcase': '...', 'result_id': '...' } ]

So then the caller knows for test-result-missing they have to figure out why the test case didn't run. For test-result-failed they have to either get it to pass or waive it. etc.

dcallagh commented on line 116 of greenwave/api_v1.py 6 years ago

If we represent (internally) the policies as a set of rules, then we should find a nice correspondence from rule type -> Matcher class -> "type" in this return value.

dcallagh commented on line 33 of greenwave/app_factory.py 6 years ago

Maybe drop this condition like we did in Waiverdb.

dcallagh commented on line 15 of greenwave/utils.py 6 years ago

Ohh I missed the fact that that other bit of code was extracting this error string from the JSON response body... hmm. Not sure what is the cleanest way to handle it. Maybe a custom version of .raise_for_status() which annotates the exception with this message, if it's present?

When does ResultsDB (or WaiverDB) return an HTTP error with a JSON message key? Does that ever happen? I can't find any mention of it in the ResultsDB API docs. Maybe we don't need to bother with this at all?

mjia commented on line 18 of greenwave/api_v1.py 6 years ago

Thanks.

mjia commented on line 33 of greenwave/api_v1.py 6 years ago

Yeah, it might be easier as if what we have done in beaker.

mjia commented on line 72 of greenwave/api_v1.py 6 years ago

Good point.

mjia commented on line 15 of greenwave/utils.py 6 years ago

The JSON message key is returned in ResultsDB, not in WaiverDB. This method is copied from https://github.com/release-engineering/resultsdb-updater/blob/master/resultsdbupdater/utils.py

mjia commented 6 years ago

IMO, switching it to a POST with JSON may make more sense since the caller is expecting the API server to make/create a decision, even though we're not storing the decision.

ralph commented 6 years ago

Me too. I'm swayed by the argument that we'll need to "express more complex values."

jcline commented 6 years ago

+1 to POSTing a JSON body

mjia commented on line 15 of greenwave/utils.py 6 years ago

Just have another look and I think you are right. We don't need to bother with this at all as the message key is only returned in POST requests in ResultsDB.

rebased

6 years ago

mjia commented 6 years ago

Rebased to address the code review comments:

change /decision to POST
add tests

rebased

6 years ago

rebased

6 years ago

rebased

6 years ago

mjia commented 6 years ago

Rebased to add the resultsdb api url and the waiverdb api url for the dev environment.

Edited 6 years ago by mjia

rebased

6 years ago

dcallagh commented on line 28 of greenwave/logger.py 6 years ago

If we are going full container, then we can probably take this stuff back out. There is no real solution for structured logging in OpenShift, it's all just plain text to stdout (a big step backwards IMHO).

dcallagh commented on line 28 of greenwave/logger.py 6 years ago

dcallagh commented on line 18 of greenwave/policies.py 6 years ago

Actually looking at the ruleset it seems there is no actual requirement that the RPMDiff results pass or are waived. :-) But we can fix that in a later patch once this is merged...

dcallagh commented on line 12 of greenwave/tests/cassettes/failed_result.yaml 6 years ago

I am not on board with this VCR cassette stuff.

It just produces these huge, unreadable blobs of YAML and if we ever need to change anything in the responses, someone has to re-record them by hand and you get a massive unreadable diff with no idea what is changing.

I think we really need to either:

write unit tests with mock Waiverdb and Resultsdb services
write functional tests which talk to a real Greenwave service deployed in OpenShift, and include ResultsDB and WaiverDB in the OpenShift template so they get deployed beside it

I am much more strongly in favour of option 2, because I always prefer tests that run the code in a realistic way as part of a real deployment, rather than faking pieces out.

But I think the key is that each test case should be specifying what setup it expects (in terms of, results and waivers existing) and then asserting the correct response comes back from Greenwave.

dcallagh commented 6 years ago

Probably the neatest approach for testing would be two-pronged:

If we introduce the idea of each policy consisting of "matcher" objects, like I floated earlier... the matchers would not be making HTTP requests, they would just deal with results and waivers in, and answers out. It should be easy to thoroughly unit test these without any HTTP request stuff.

And then on the outer layer, we have functional tests which interact with a real ResultsDB+WaiverDB+Greenwave in OpenShift. The test would POST some results to ResultsDB, POST some waivers to WaiverDB, and then ask Greenwave for a decision and assert it is correct.

We would not need too many heavy-weight functional tests because all the edge cases for individual policies could be covered by unit tests against the "matchers".

... But in the interest of keeping this pull request small, maybe let's leave out the tests entirely from this one and we can iterate on them separately? For the functional testing I guess we would want a Jenkinsfile, .spec file, Dockerfile, and OpenShift templates similar to WaiverDB...

mikeb commented 6 years ago

Re: specifying what setup a testcase expects, I wonder if this would be a good use-case for custom Ansible modules. You would specify in yaml the data that should exist for each service, and the modules would handle the specifics of getting that data into the service, whether it's direct database inserts, XMLRPC POSTs, REST calls, etc. These modules could be reused for setup/bootstrapping of production services as well.

mjia commented on line 28 of greenwave/logger.py 6 years ago

Well, I would say it doesn't hurt to have this here.

Edited 6 years ago by mjia

mjia commented on line 12 of greenwave/tests/cassettes/failed_result.yaml 6 years ago

The VCR.py is being used in MBS and resutlsdb-updater. It states that it can simplify and speed up tests that make HTTP requests. However, it has a trade-off which would include some unreadable chunks of YAML. For each test, it does specify what data it expects from ResultsDB and WaiverDB, though it is not quite readable in the YAML file. But yeah, it might be overkill here.

The option 2 sounds great, but I think we can go with the option 1 for now as a quick solution.

rebased

6 years ago

mjia commented 6 years ago

I've changed to use requests-mock instead of the VCR.py for the tests. I would like to keep the tests here in order to prove that the code actually works as expected. We could refactor the code when the idea of using "matcher" objects in the policies is introduced.

dcallagh commented 6 years ago

Ahh very nice! I greatly prefer this requests_mock approach over the vcrpy stuff. It makes it easy to see in each test case what fake HTTP requests we are relying on, and to change them as the tests evolve.

And I agree, we can refactor the policies further and improve the tests in subsequent PRs. This is a good starting point.

:+1: from me to merge this!

ralph commented 6 years ago

@mjia any reason not to merge this?

Do you have sufficient rights in pagure?

jcline commented on line 63 of greenwave/api_v1.py 6 years ago

This approach defeats the purpose of using a session since the session is discarded after each request is handled. The session must live beyond the request/response cycle.

jcline commented on line 36 of greenwave/api_v1.py 6 years ago

I know there was a decision to not use a REST framework, but this schema validation could be handled in a much more readable way with any number of libraries. Something like marshmallow (which, incidentally, flask-restful is going to start recommending) would do this and it pulls the schema definition/validation out of the request handler.

Just a thought.

jcline commented on line 69 of greenwave/api_v1.py 6 years ago

I don't see any error handling code for the exception this raises, so the response is likely an HTTP 500 rather than a more useful error message about the service being unavailable or something.

The cleanest way to handle this likely with an error handler for the various requests exception types.

jcline commented on line 23 of greenwave/api_v1.py 6 years ago

I suggest refactoring this function into several smaller functions that the route handler ties together. It's difficult to follow due to the multiple nested loops and if statements.

mjia commented on line 23 of greenwave/api_v1.py 6 years ago

Yeah, I believe this will more likely be changed when more policies are added. I would like to keep it like that for the first version.

mjia commented on line 36 of greenwave/api_v1.py 6 years ago

Marshmallow might be a good option if we have many APIs doing this like that. For this small app, it might be overkill. I would rather improve the error messages to be more readable.

mjia commented on line 63 of greenwave/api_v1.py 6 years ago

I'm not quite sure what you mean since I'm using a context manager here which creates a global session. It will be closed once all the requests inside the context manager are handled.

mjia commented on line 69 of greenwave/api_v1.py 6 years ago

Yeah, I will fix this in the following PRs.

mjia commented 6 years ago