PR#99: Waive the absence of a result and dummy auth for cli - waiverdb

waiverdb

#99 Waive the absence of a result and dummy auth for cli

Closed 6 years ago by ralph. Opened 6 years ago by gnaponie.

gnaponie/waiverdb noresult into master

Added dummy auth for CLI

Giulia Naponiello • 6 years ago

68e2b2e

Provide a way to waive the absence of a test result

Giulia Naponiello • 6 years ago

3662ec2

requirements.txt

file modified

		`@@ -11,6 +11,7 @@`
		`kerberos >= 1.1.1`
		`flask-oidc`
		`systemd`
		`+ sqlalchemy-utils`
		`# packages for the unit tests`
		`pytest >= 2.4.2`
		`mock`

tests/test_api_v10.py

file modified

+155 -47

		`@@ -11,7 +11,8 @@`
		`@patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`def test_create_waiver(mocked_get_user, client, session):`
		`data = {`
		`- 'result_id': 123,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'product_version': 'fool-1',`
		`'waived': True,`
		`'comment': 'it broke',`
		`@@ -21,28 +22,43 @@`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 201`
		`assert res_data['username'] == 'foo'`
		`- assert res_data['result_id'] == 123`
		`+ assert res_data['subject'] == {'subject.test': 'subject'}`
		`+ assert res_data['testcase'] == 'testcase1'`
		`assert res_data['product_version'] == 'fool-1'`
		`assert res_data['waived'] is True`
		`assert res_data['comment'] == 'it broke'`


		`@patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`- def test_create_waiver_with_malformed_data(mocked_get_user, client):`
		`+ def test_create_waiver_with_no_testcase(mocked_get_user, client):`
		`data = {`
		`- 'result_id': 'wrong id',`
		`+ 'subject': {'foo': 'bar'},`
		`}`
		`r = client.post('/api/v1.0/waivers/', data=json.dumps(data),`
		`content_type='application/json')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 400`
		`- assert 'invalid literal for int()' in res_data['message']['result_id']`
		`+ assert 'Missing required parameter in the JSON body' in res_data['message']['testcase']`
		`+`
		`+`
		`+ @patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`+ def test_create_waiver_with_malformed_subject(mocked_get_user, client):`
		`+ data = {`
		`+ 'subject': 'asd',`
		`+ 'testcase': 'qqq',`
		`+ }`
		`+ r = client.post('/api/v1.0/waivers/', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 400`
		`+ assert 'Must be a valid dict' in res_data['message']['subject']`


		`@patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`def test_non_superuser_cannot_create_waiver_for_other_users(mocked_get_user, client):`
		`data = {`
		`- 'result_id': 123,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'product_version': 'fool-1',`
		`'waived': True,`
		`'comment': 'it broke',`
		`@@ -58,7 +74,8 @@`
		`@patch('waiverdb.auth.get_user', return_value=('bodhi', {}))`
		`def test_superuser_can_create_waiver_for_other_users(mocked_get_user, client, session):`
		`data = {`
		`- 'result_id': 123,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'product_version': 'fool-1',`
		`'waived': True,`
		`'comment': 'it broke',`
		`@@ -75,13 +92,15 @@`

		`def test_get_waiver(client, session):`
		`# create a new waiver`
		`- waiver = create_waiver(session, result_id=123, username='foo',`
		`+ waiver = create_waiver(session, subject={'subject.test': 'subject'},`
		`+ testcase='testcase1', username='foo',`
		`product_version='foo-1', comment='bla bla bla')`
		`r = client.get('/api/v1.0/waivers/%s' % waiver.id)`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert res_data['username'] == waiver.username`
		`- assert res_data['result_id'] == waiver.result_id`
		`+ assert res_data['subject'] == waiver.subject`
		`+ assert res_data['testcase'] == waiver.testcase`
		`assert res_data['product_version'] == waiver.product_version`
		`assert res_data['waived'] is True`
		`assert res_data['comment'] == waiver.comment`
		`@@ -107,7 +126,8 @@`

		`def test_get_waivers(client, session):`
		`for i in range(0, 10):`
		`- create_waiver(session, result_id=i, username='foo %d' % i,`
		`+ create_waiver(session, subject={"subject%d" % i: "%d" % i},`
		`+ testcase="case %d" % i, username='foo %d' % i,`
		`product_version='foo-%d' % i, comment='bla bla bla')`
		`r = client.get('/api/v1.0/waivers/')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`@@ -117,7 +137,8 @@`

		`def test_pagination_waivers(client, session):`
		`for i in range(0, 30):`
		`- create_waiver(session, result_id=i, username='foo %d' % i,`
		`+ create_waiver(session, subject={"subject%d" % i: "%d" % i},`
		`+ testcase="case %d" % i, username='foo %d' % i,`
		`product_version='foo-%d' % i, comment='bla bla bla')`
		`r = client.get('/api/v1.0/waivers/?page=2')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`@@ -130,9 +151,11 @@`


		`def test_obsolete_waivers_are_excluded_by_default(client, session):`
		`- create_waiver(session, result_id=123, username='foo',`
		`+ create_waiver(session, subject={'subject.test': 'subject'},`
		`+ testcase='testcase1', username='foo',`
		`product_version='foo-1')`
		`- new_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ new_waiver = create_waiver(session, subject={'subject.test': 'subject'},`
		`+ testcase='testcase1', username='foo',`
		`product_version='foo-1', waived=False)`
		`r = client.get('/api/v1.0/waivers/')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`@@ -143,9 +166,11 @@`


		`def test_get_obsolete_waivers(client, session):`
		`- old_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ old_waiver = create_waiver(session, subject={'subject.test': 'subject'},`
		`+ testcase='testcase1', username='foo',`
		`product_version='foo-1')`
		`- new_waiver = create_waiver(session, result_id=123, username='foo',`
		`+ new_waiver = create_waiver(session, subject={'subject.test': 'subject'},`
		`+ testcase='testcase1', username='foo',`
		`product_version='foo-1', waived=False)`
		`r = client.get('/api/v1.0/waivers/?include_obsolete=1')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`@@ -155,31 +180,73 @@`
		`assert res_data['data'][1]['id'] == old_waiver.id`


		`- def test_filtering_waivers_by_result_id(client, session):`
		`- create_waiver(session, result_id=123, username='foo-1', product_version='foo-1')`
		`- create_waiver(session, result_id=234, username='foo-2', product_version='foo-1')`
		`- r = client.get('/api/v1.0/waivers/?result_id=123')`
		`+ def test_filtering_waivers_by_subject_and_testcase(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='foo-2', product_version='foo-1')`
		`+`
		`+ param = json.dumps([{'subject': {'subject.test1': 'subject1'}, 'testcase': 'testcase1'}])`
		`+ r = client.get('/api/v1.0/waivers/?results=%s' % param)`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 1`
		`- assert res_data['data'][0]['result_id'] == 123`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`


		`- def test_filtering_waivers_by_multiple_result_ids(client, session):`
		`- create_waiver(session, result_id=123, username='foo-1', product_version='foo-1')`
		`- create_waiver(session, result_id=234, username='foo-2', product_version='foo-1')`
		`- create_waiver(session, result_id=345, username='foo-2', product_version='foo-1')`
		`- r = client.get('/api/v1.0/waivers/?result_id=123,345')`
		`+ def test_filtering_waivers_by_subject_and_testcase_with_other_json_order(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='foo-1')`
		`+`
		`+ param = json.dumps([{'testcase': 'testcase1', 'subject': {'subject.test1': 'subject1'}}])`
		`+ r = client.get('/api/v1.0/waivers/?results=%s' % param)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`
		`+`
		`+`
		`+ def test_filtering_waivers_by_multiple_results_subjects_and_testcases(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='foo-2', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test3': 'subject3'},`
		`+ testcase='testcase3', username='foo-2', product_version='foo-1')`
		`+ param = json.dumps([{'subject': {'subject.test1': 'subject1'}, 'testcase': 'testcase1'},`
		`+ {'subject': {'subject.test2': 'subject2'}, 'testcase': 'testcase2'}])`
		`+ r = client.get('/api/v1.0/waivers/?results=%s' % param)`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 2`
		`- assert res_data['data'][0]['result_id'] == 345`
		`- assert res_data['data'][1]['result_id'] == 123`
		`+ assert res_data['data'][0]['subject'] == {'subject.test2': 'subject2'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase2'`
		`+ assert res_data['data'][1]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][1]['testcase'] == 'testcase1'`
		`+`
		`+`
		`+ def test_filtering_waivers_by_subject_without_testcase(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='foo-2', product_version='foo-1')`
		`+`
		`+ param = json.dumps([{'subject': {'subject.test1': 'subject1'}}])`
		`+ r = client.get('/api/v1.0/waivers/?results=%s' % param)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 200`
		`+ assert len(res_data['data']) == 1`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`


		`def test_filtering_waivers_by_product_version(client, session):`
		`- create_waiver(session, result_id=123, username='foo-1', product_version='release-1')`
		`- create_waiver(session, result_id=124, username='foo-1', product_version='release-2')`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='release-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='foo-1', product_version='release-2')`
		`r = client.get('/api/v1.0/waivers/?product_version=release-1')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`@@ -188,8 +255,10 @@`


		`def test_filtering_waivers_by_username(client, session):`
		`- create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`- create_waiver(session, result_id=124, username='bar', product_version='foo-2')`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='bar', product_version='foo-2')`
		`r = client.get('/api/v1.0/waivers/?username=foo')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`@@ -201,18 +270,21 @@`
		`before1 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=100)).isoformat()`
		`before2 = (datetime.datetime.utcnow() - datetime.timedelta(seconds=99)).isoformat()`
		`after = (datetime.datetime.utcnow() + datetime.timedelta(seconds=100)).isoformat()`
		`- create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo', product_version='foo-1')`
		`r = client.get('/api/v1.0/waivers/?since=%s' % before1)`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 1`
		`- assert res_data['data'][0]['result_id'] == 123`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`

		`r = client.get('/api/v1.0/waivers/?since=%s,%s' % (before1, after))`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 1`
		`- assert res_data['data'][0]['result_id'] == 123`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`

		`r = client.get('/api/v1.0/waivers/?since=%s' % (after))`
		`res_data = json.loads(r.get_data(as_text=True))`
		`@@ -225,19 +297,33 @@`
		`assert len(res_data['data']) == 0`


		`+ def test_filtering_waivers_by_malformed_since(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo', product_version='foo-1')`
		`+ wrong_since = 123`
		`+ r = client.get('/api/v1.0/waivers/?since=%s' % wrong_since)`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 400`
		`+ assert res_data['message'] == "'since' parameter not in ISO8601 format"`
		`+`
		`+`
		`def test_filtering_waivers_by_proxied_by(client, session):`
		`- create_waiver(session, result_id=123, username='foo-1', product_version='foo-1',`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo-1', product_version='foo-1',`
		`proxied_by='bodhi')`
		`- create_waiver(session, result_id=234, username='foo-2', product_version='foo-1')`
		`+ create_waiver(session, subject={'subject.test2': 'subject2'},`
		`+ testcase='testcase2', username='foo-2', product_version='foo-1')`
		`r = client.get('/api/v1.0/waivers/?proxied_by=bodhi')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 1`
		`- assert res_data['data'][0]['result_id'] == 123`
		`+ assert res_data['data'][0]['subject'] == {'subject.test1': 'subject1'}`
		`+ assert res_data['data'][0]['testcase'] == 'testcase1'`


		`def test_jsonp(client, session):`
		`- waiver = create_waiver(session, result_id=123, username='foo', product_version='foo-1')`
		`+ waiver = create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo', product_version='foo-1')`
		`r = client.get('/api/v1.0/waivers/%s?callback=jsonpcallback' % waiver.id)`
		`assert r.mimetype == 'application/javascript'`
		`assert 'jsonpcallback' in r.get_data(as_text=True)`
		`@@ -252,23 +338,45 @@`
		`def test_get_waivers_with_post_request(client, session):`
		`"""`
		`This tests that users can get waivers by sending a POST request with a long`
		`- list of result ids.`
		`+ list of result subject/testcase.`
		`"""`
		`- result_ids = []`
		`+ results = []`
		`for i in range(1, 51):`
		`- result_ids.append(str(i))`
		`- create_waiver(session, result_id=i, username='foo', product_version='foo-1')`
		`+ results.append({'subject': {'subject%d' % i: '%d' % i},`
		`+ 'testcase': 'case %d' % i})`
		`+ create_waiver(session, subject={"subject%d" % i: "%d" % i},`
		`+ testcase="case %d" % i, username='foo %d' % i,`
		`+ product_version='foo-%d' % i, comment='bla bla bla')`
		`data = {`
		`- 'result_ids': result_ids`
		`+ 'results': results`
		`}`
		`- r = client.post('/api/v1.0/waivers/+by-result-ids', data=json.dumps(data),`
		`+ r = client.post('/api/v1.0/waivers/+by-subjects-and-testcases', data=json.dumps(data),`
		`content_type='application/json')`
		`res_data = json.loads(r.get_data(as_text=True))`
		`assert r.status_code == 200`
		`assert len(res_data['data']) == 50`
		`- assert set([w['result_id'] for w in res_data['data']]) == set(range(1, 51))`
		`- assert all(w['username'] == 'foo' for w in res_data['data'])`
		`- assert all(w['product_version'] == 'foo-1' for w in res_data['data'])`
		`+ subjects = []`
		`+ testcases = []`
		`+ for i in reversed(range(1, 51)):`
		`+ subjects.append({'subject%d' % i: '%d' % i})`
		`+ testcases.append('case %d' % i)`
		`+ assert [w['subject'] for w in res_data['data']] == subjects`
		`+ assert set([w['testcase'] for w in res_data['data']]) == set(testcases)`
		`+ assert all(w['username'].startswith('foo') for w in res_data['data'])`
		`+ assert all(w['product_version'].startswith('foo-') for w in res_data['data'])`
		`+`
		`+`
		`+ def test_get_waivers_with_post_malformed_since(client, session):`
		`+ create_waiver(session, subject={'subject.test1': 'subject1'},`
		`+ testcase='testcase1', username='foo', product_version='foo-1')`
		`+ data = {`
		`+ 'since': 123`
		`+ }`
		`+ r = client.post('/api/v1.0/waivers/+by-subjects-and-testcases', data=json.dumps(data),`
		`+ content_type='application/json')`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert r.status_code == 400`
		`+ assert res_data['message'] == "'since' parameter not in ISO8601 format"`


		`def test_about_endpoint(client):`

tests/test_auth.py

file modified

+2 -1

		`@@ -38,7 +38,8 @@`
		`client, monkeypatch, session):`
		`monkeypatch.setenv('KRB5_KTNAME', '/etc/foo.keytab')`
		`data = {`
		`- 'result_id': 123,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'product_version': 'fool-1',`
		`'waived': True,`
		`'comment': 'it broke',`

tests/test_cli.py

file modified

+32 -9

		`@@ -95,7 +95,7 @@`
		`assert result.output == 'Error: Please specify product version\n'`


		`- def test_no_result_id(tmpdir):`
		`+ def test_no_subject(tmpdir):`
		`p = tmpdir.join('client.conf')`
		`p.write("""`
		`[waiverdb]`
		`@@ -110,7 +110,25 @@`
		`args = ['-C', p.strpath, '-p', 'fedora-26']`
		`result = runner.invoke(waiverdb_cli, args)`
		`assert result.exit_code == 1`
		`- assert result.output == 'Error: Please specify one or more result ids to waive\n'`
		`+ assert result.output == 'Error: Please specify one subject\n'`
		`+`
		`+`
		`+ def test_no_testcase(tmpdir):`
		`+ p = tmpdir.join('client.conf')`
		`+ p.write("""`
		`+ [waiverdb]`
		`+ auth_method=OIDC`
		`+ api_url=http://localhost:5004/api/v1.0`
		`+ oidc_id_provider=https://id.stg.fedoraproject.org/openidc/`
		`+ oidc_client_id=waiverdb`
		`+ oidc_scopes=`
		`+ openid`
		`+ """)`
		`+ runner = CliRunner()`
		`+ args = ['-C', p.strpath, '-p', 'fedora-26', '-s', 'subject']`
		`+ result = runner.invoke(waiverdb_cli, args)`
		`+ assert result.exit_code == 1`
		`+ assert result.output == 'Error: Please specify testcase\n'`


		`def test_oidc_auth_is_enabled(tmpdir):`
		`@@ -123,7 +141,8 @@`
		`"comment": "It's dead!",`
		`"id": 15,`
		`"product_version": "Parrot",`
		`- "result_id": 123,`
		`+ "subject": {"subject.test": "test", "s": "t"},`
		`+ "testcase": "test.testcase",`
		`"timestamp": "2017-010-16T17:42:04.209638",`
		`"username": "foo",`
		`"waived": True`
		`@@ -140,10 +159,12 @@`
		`openid`
		`""")`
		`runner = CliRunner()`
		`- args = ['-C', p.strpath, '-p', 'Parrot', '-r', 123, '-c', "It's dead!"]`
		`+ args = ['-C', p.strpath, '-p', 'Parrot', '-s', '{"subject.test": "test", "s": "t"}',`
		`+ '-t', 'test.testcase', '-c', "It's dead!"]`
		`result = runner.invoke(waiverdb_cli, args)`
		`exp_json = {`
		`- 'result_id': 123,`
		`+ "subject": {"subject.test": "test", "s": "t"},`
		`+ "testcase": "test.testcase",`
		`'waived': True,`
		`'product_version': 'Parrot',`
		`'comment': "It's dead!"`
		`@@ -155,7 +176,7 @@`
		`timeout=60,`
		`headers={'Content-Type': 'application/json'})`
		`assert result.exit_code == 0`
		`- assert result.output == 'Created waiver 15 for result 123\n'`
		`+ assert result.output == 'Created waiver 15 for result with subject {"subject.test": "test", "s": "t"} and testcase test.testcase\n' # noqa`


		`def test_kerberos_is_enabled(tmpdir):`
		`@@ -168,7 +189,8 @@`
		`"comment": "It's dead!",`
		`"id": 15,`
		`"product_version": "Parrot",`
		`- "result_id": 123,`
		`+ "subject": {"subject.test": "test", "s": "t"},`
		`+ "testcase": "test.testcase",`
		`"timestamp": "2017-010-16T17:42:04.209638",`
		`"username": "foo",`
		`"waived": True`
		`@@ -181,7 +203,8 @@`
		`api_url=http://localhost:5004/api/v1.0`
		`""")`
		`runner = CliRunner()`
		`- args = ['-C', p.strpath, '-p', 'Parrot', '-r', 123, '-c', "It's dead!"]`
		`+ args = ['-C', p.strpath, '-p', 'Parrot', '-s', '{"subject.test": "test", "s": "t"}',`
		`+ '-t', 'test.testcase', '-c', "It's dead!"]`
		`result = runner.invoke(waiverdb_cli, args)`
		`mock_request.assert_called_once()`
		`- assert result.output == 'Created waiver 15 for result 123\n'`
		`+ assert result.output == 'Created waiver 15 for result with subject {"subject.test": "test", "s": "t"} and testcase test.testcase\n' # noqa`

tests/test_events.py

file modified

+8 -4

		`@@ -9,7 +9,8 @@`
		`@mock.patch('waiverdb.events.fedmsg')`
		`def test_publish_new_waiver_with_fedmsg(mock_fedmsg, session):`
		`waiver = Waiver(`
		`- result_id=1,`
		`+ subject={'subject.test': 'subject'},`
		`+ testcase='testcase1',`
		`username='jcline',`
		`product_version='something',`
		`waived=True,`
		`@@ -22,7 +23,8 @@`
		`topic='waiver.new',`
		`msg={`
		`'id': waiver.id,`
		`- 'result_id': 1,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'username': 'jcline',`
		`'proxied_by': None,`
		`'product_version': 'something',`
		`@@ -36,7 +38,8 @@`
		`@mock.patch('waiverdb.events.fedmsg')`
		`def test_publish_new_waiver_with_fedmsg_for_proxy_user(mock_fedmsg, session):`
		`waiver = Waiver(`
		`- result_id=1,`
		`+ subject={'subject.test': 'subject'},`
		`+ testcase='testcase1',`
		`username='jcline',`
		`product_version='something',`
		`waived=True,`
		`@@ -50,7 +53,8 @@`
		`topic='waiver.new',`
		`msg={`
		`'id': waiver.id,`
		`- 'result_id': 1,`
		`+ 'subject': {'subject.test': 'subject'},`
		`+ 'testcase': 'testcase1',`
		`'username': 'jcline',`
		`'proxied_by': 'bodhi',`
		`'product_version': 'something',`

tests/utils.py

file modified

+3 -2

		`@@ -3,9 +3,10 @@`
		`from waiverdb.models import Waiver`


		`- def create_waiver(session, result_id, username, product_version, waived=True,`
		`+ def create_waiver(session, subject, testcase, username, product_version, waived=True,`
		`comment=None, proxied_by=None):`
		`- waiver = Waiver(result_id, username, product_version, waived, comment, proxied_by)`
		`+ waiver = Waiver(subject, testcase, username, product_version, waived, comment,`
		`+ proxied_by)`
		`session.add(waiver)`
		`session.flush()`
		`return waiver`

waiverdb/api_v1.py

file modified

+86 -30

		`@@ -1,8 +1,10 @@`
		`# SPDX-License-Identifier: GPL-2.0+`

		`+ import json`
		`+`
		`from flask import Blueprint, request, current_app`
		`from flask_restful import Resource, Api, reqparse, marshal_with, marshal`
		`- from werkzeug.exceptions import BadRequest, NotFound, UnsupportedMediaType, Forbidden`
		`+ from werkzeug.exceptions import BadRequest, UnsupportedMediaType, Forbidden`
		`from sqlalchemy.sql.expression import func`

		`from waiverdb import __version__`
		`@@ -14,19 +16,27 @@`
		`api_v1 = (Blueprint('api_v1', __name__))`
		`api = Api(api_v1)`

		`+`
		`+ def valid_dict(value):`
		`+ if not isinstance(value, dict):`
		`+ raise ValueError("Must be a valid dict, not %r" % value)`
		`+ return value`
		`+`
		`+`
		`# RP contains request parsers (reqparse.RequestParser).`
		`# Parsers are added in each 'resource section' for better readability`
		`RP = {}`

		`RP['create_waiver'] = reqparse.RequestParser()`
		`- RP['create_waiver'].add_argument('result_id', type=int, required=True, location='json')`
		`+ RP['create_waiver'].add_argument('subject', type=valid_dict, required=True, location='json')`
		`+ RP['create_waiver'].add_argument('testcase', type=str, required=True, location='json')`
		`RP['create_waiver'].add_argument('waived', type=bool, required=True, location='json')`
		`RP['create_waiver'].add_argument('product_version', type=str, required=True, location='json')`
		`RP['create_waiver'].add_argument('comment', type=str, default=None, location='json')`
		`RP['create_waiver'].add_argument('username', type=str, default=None, location='json')`

		`RP['get_waivers'] = reqparse.RequestParser()`
		`- RP['get_waivers'].add_argument('result_id', location='args')`
		`+ RP['get_waivers'].add_argument('results', location='args')`
		`RP['get_waivers'].add_argument('product_version', location='args')`
		`RP['get_waivers'].add_argument('username', location='args')`
		`RP['get_waivers'].add_argument('include_obsolete', type=bool, default=False, location='args')`
		`@@ -74,8 +84,8 @@`

		`:query int page: The page to get.`
		`:query int limit: Limit the number of items returned.`
		`- :query int result_id: Filter the waivers by result ID. Accepts one or`
		`- more result IDs separated by commas.`
		`+ :query string results: Filter the waivers by result. Accepts a list of`
dcallagh commented 6 years ago This API doesn't make sense to me... not sure if I am just behind the times. But I thought that we changed it so that a waiver is recorded for a subject and testcase, right? So if this is for selecting out waivers, and optionally filtering them, then surely it should just be two parameters (both optional), 'subject' and 'testcase' which filter the waivers down the requested value if the parameter is present. This thing where it has to be a JSON-encoded array of dicts with 'subject' and 'testcase' seems very complex and difficult for the callers to use. Who are the callers anyway?
mjia commented 6 years ago Good point. As far as I know, no caller is calling this right now.
		`+ dictionaries, with one key 'subject' and one key 'testcase'.`
		`:query string product_version: Filter the waivers by product version.`
		`:query string username: Filter the waivers by username.`
		`:query string proxied_by: Filter the waivers by the users who are`
		`@@ -91,8 +101,19 @@`
		`"""`
		`args = RP['get_waivers'].parse_args()`
		`query = Waiver.query.order_by(Waiver.timestamp.desc())`
		`- if args['result_id']:`
		`- query = query.filter(Waiver.result_id.in_(args['result_id'].split(',')))`
		`+`
		`+ if args['results']:`
		`+ results = json.loads(args['results'])`
		`+ for d in results:`
		`+ if d.get('subject', None):`
mjia commented 6 years ago You do not need to have two ifs here as you can just do: if not isinstance(d.get('subject', None), dict):
gnaponie commented 6 years ago mmm I think at this point there was some confusion (in my head at least...). You can filter without a subject... So if the user makes a request without specifying a subject (for example), this: not isinstance(d.get('subject', None), dict) would be --> d.get('subject', None) will be None not isinstance(d.get('subject', None), dict) will be True so you will have a Bad request... even if you can filter without a subject (or testcase, or both). Because I think the error message (for the bad request) should say to the user that he used the wrong format... and not that the parameter is missing. Does it make sense?
mjia commented 6 years ago It seems like we are going to change the API so I guess this part wouldn't be needed anymore.
		`+ if not isinstance(d.get('subject', None), dict):`
		`+ raise BadRequest("'results' parameter should be a list \`
		`+ of dictionaries with subject and testcase")`
		`+ if d.get('testcase', None):`
mjia commented 6 years ago Same as above.
		`+ if not isinstance(d.get('testcase', None), basestring):`
		`+ raise BadRequest("'results' parameter should be a list \`
mjia commented 6 years ago Maybe we should give a clear message like "testcase is missing in 'results' parameter"
		`+ of dictionaries with subject and testcase")`
		`+ query = Waiver.by_results(query, results)`
		`if args['product_version']:`
		`query = query.filter(Waiver.product_version == args['product_version'])`
		`if args['username']:`
		`@@ -102,15 +123,19 @@`
		`if args['since']:`
		`try:`
		`since_start, since_end = reqparse_since(args['since'])`
		`- except:`
		`+ except ValueError:`
		`+ raise BadRequest("'since' parameter not in ISO8601 format")`
		`+ except TypeError:`
		`raise BadRequest("'since' parameter not in ISO8601 format")`
		`if since_start:`
		`query = query.filter(Waiver.timestamp >= since_start)`
		`if since_end:`
		`query = query.filter(Waiver.timestamp <= since_end)`
		`if not args['include_obsolete']:`
		`- subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.result_id)`
		`+ subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.subject,`
		`+ Waiver.testcase)`
		`query = query.filter(Waiver.id.in_(subquery))`
		`+ query = query.order_by(Waiver.timestamp.desc())`
		`return json_collection(query, args['page'], args['limit'])`

		`@jsonp`
		`@@ -133,7 +158,8 @@`
		`Content-Length: 91`

		`{`
		`- "result_id": 1,`
		`+ "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"},`
		`+ "testcase": "compose.install_no_user",`
		`"waived": false,`
		`"product_version": "Parrot",`
		`"comment": "It's dead!"`
		`@@ -155,20 +181,23 @@`
		`"comment": "It's dead!",`
		`"id": 15,`
		`"product_version": "Parrot",`
		`- "result_id": 1,`
		`+ "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"},`
		`+ "testcase": "compose.install_no_user",`
		`"timestamp": "2017-03-16T17:42:04.209638",`
		`"username": "jcline",`
		`"waived": false,`
		`"proxied_by": null`
		`}`

		`- :json int result_id: The result ID for the waiver.`
		`+ :json object subject: The result subject for the waiver.`
		`+ :json string testcase: The result testcase for the waiver.`
		`:json boolean waived: Whether or not the result is waived.`
		`:json string product_version: The product version string.`
		`:json string comment: A comment explaining the waiver.`
		`:json string username: Username on whose behalf the caller is proxying.`
		`:statuscode 201: The waiver was successfully created.`
		`"""`
		`+`
		`user, headers = waiverdb.auth.get_user(request)`
		`args = RP['create_waiver'].parse_args()`
		`proxied_by = None`
		`@@ -177,8 +206,8 @@`
		`raise Forbidden('user %s does not have the proxyuser ability' % user)`
		`proxied_by = user`
		`user = args['username']`
		`- waiver = Waiver(args['result_id'], user, args['product_version'], args['waived'],`
		`- args['comment'], proxied_by)`
		`+ waiver = Waiver(args['subject'], args['testcase'], user,`
		`+ args['product_version'], args['waived'], args['comment'], proxied_by)`
		`db.session.add(waiver)`
		`db.session.commit()`
		`return waiver, 201, headers`
		`@@ -198,24 +227,25 @@`
		`"""`
		`try:`
		`return Waiver.query.get_or_404(waiver_id)`
		`- except:`
		`+ except Exception as NotFound:`
		`raise NotFound('Waiver not found')`


		`- class GetWaiversByResultIDs(Resource):`
		`+ class GetWaiversBySubjectsAndTestcases(Resource):`
		`@jsonp`
		`def post(self):`
		`"""`
		`- Return a list of waivers by filtering the waivers with a list of result ids.`
		`- This accepts POST requests in order to handle a special case where a`
		`- GET /waivers/ request has a long query string with many result ids that`
		`- could cause 413 erros.`
		`+ Return a list of waivers by filtering the waivers with a`
		`+ list of result subjects and testcases.`
		`+ This accepts POST requests in order to handle a special`
		`+ case where a GET /waivers/ request has a long query string with many`
		`+ result subjects/testcases that could cause 413 errors.`

		`Sample request:`

		`.. sourcecode:: http`

		`- POST /api/v1.0/waivers/+by-result-ids HTTP/1.1`
		`+ POST /api/v1.0/waivers/+by-subjects-and-testcases HTTP/1.1`
		`Host: localhost:5004`
		`Accept-Encoding: gzip, deflate`
		`Accept: application/json`
		`@@ -225,7 +255,16 @@`
		`Content-Length: 40`

		`{`
		`- "result_ids": [1,2]`
		`+ "results": [`
		`+ {`
		`+ "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"},`
		`+ "testcase": "compose.install_no_user"`
		`+ },`
		`+ {`
		`+ "subject": {"item": "gzip-1.9-1.fc28", "type": "koji_build"},`
		`+ "testcase": "dist.rpmlint"`
		`+ }`
		`+ ]`
		`}`

		`Sample response:`
		`@@ -244,7 +283,8 @@`
		`"comment": "It's dead!",`
		`"id": 5,`
		`"product_version": "Parrot",`
		`- "result_id": 2,`
		`+ "subject": {"productmd.compose.id": "Fedora-9000-19700101.n.18"},`
		`+ "testcase": "compose.install_no_user",`
		`"timestamp": "2017-09-21T04:55:53.343368",`
		`"username": "dummy",`
		`"waived": true,`
		`@@ -254,7 +294,8 @@`
		`"comment": "It's dead!",`
		`"id": 4,`
		`"product_version": "Parrot",`
		`- "result_id": 1,`
		`+ "subject": {"item": "gzip-1.9-1.fc28", "type": "koji_build"},`
		`+ "testcase": "dist.rpmlint",`
		`"timestamp": "2017-09-21T04:55:51.936658",`
		`"username": "dummy",`
		`"waived": true,`
		`@@ -263,7 +304,8 @@`
		`]`
		`}`

		`- :jsonparam array result_ids: Filter the waivers by a list of result IDs.`
		`+ :jsonparam array results: Filter the waivers by a list of dictionaries`
		`+ with result subjects and testcase.`
		`:jsonparam string product_version: Filter the waivers by product version.`
		`:jsonparam string username: Filter the waivers by username.`
		`:jsonparam string proxied_by: Filter the waivers by the users who are`
		`@@ -280,8 +322,17 @@`
		`raise UnsupportedMediaType('No JSON payload in request')`
		`data = request.get_json()`
		`query = Waiver.query.order_by(Waiver.timestamp.desc())`
		`- if 'result_ids' in data and data['result_ids']:`
		`- query = query.filter(Waiver.result_id.in_(data['result_ids']))`
		`+ if data.get('results'):`
		`+ for d in data['results']:`
		`+ if d.get('subject', None):`
mjia commented 6 years ago Same as above, you can just remove this line.
		`+ if not isinstance(d.get('subject', None), dict):`
		`+ raise BadRequest("'results' parameter should be a list \`
		`+ of dictionaries with subject and testcase")`
		`+ if d.get('testcase', None):`
mjia commented 6 years ago Same as this one.
		`+ if not isinstance(d.get('testcase', None), basestring):`
		`+ raise BadRequest("'results' parameter should be a list \`
		`+ of dictionaries with subject and testcase")`
		`+ query = Waiver.by_results(query, data['results'])`
		`if 'product_version' in data:`
		`query = query.filter(Waiver.product_version == data['product_version'])`
		`if 'username' in data:`
		`@@ -291,15 +342,20 @@`
		`if 'since' in data:`
		`try:`
		`since_start, since_end = reqparse_since(data['since'])`
		`- except:`
		`+ except ValueError:`
		`+ raise BadRequest("'since' parameter not in ISO8601 format")`
		`+ except TypeError:`
		`raise BadRequest("'since' parameter not in ISO8601 format")`
		`if since_start:`
		`query = query.filter(Waiver.timestamp >= since_start)`
		`if since_end:`
		`query = query.filter(Waiver.timestamp <= since_end)`
		`if not data.get('include_obsolete', False):`
		`- subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.result_id)`
		`+ subquery = db.session.query(func.max(Waiver.id)).group_by(Waiver.subject,`
		`+ Waiver.testcase)`
		`query = query.filter(Waiver.id.in_(subquery))`
		`+`
		`+ query = query.order_by(Waiver.timestamp.desc())`
		`return {'data': marshal(query.all(), waiver_fields)}`


		`@@ -333,5 +389,5 @@`
		`# set up the Api resource routing here`
		`api.add_resource(WaiversResource, '/waivers/')`
		`api.add_resource(WaiverResource, '/waivers/<int:waiver_id>')`
		`- api.add_resource(GetWaiversByResultIDs, '/waivers/+by-result-ids')`
		`+ api.add_resource(GetWaiversBySubjectsAndTestcases, '/waivers/+by-subjects-and-testcases')`
		`api.add_resource(AboutResource, '/about')`

waiverdb/cli.py

file modified

+51 -60

		`@@ -20,7 +20,7 @@`
		`if not config.has_option('waiverdb', 'auth_method'):`
		`raise click.ClickException(config_error.format('auth_method'))`
		`auth_method = config.get('waiverdb', 'auth_method')`
		`- if auth_method not in ['OIDC', 'Kerberos']:`
		`+ if auth_method not in ['OIDC', 'Kerberos', 'dummy']:`
		`raise click.ClickException('The WaiverDB authentication mechanism of '`
		`'"{0}" is not supported'.format(auth_method))`
		`required_configs = ['api_url']`
		`@@ -37,36 +37,48 @@`
		`@click.option('--config-file', '-C', default='/etc/waiverdb/client.conf',`
		`type=click.Path(exists=True),`
		`help='Specify a config file to use')`
		`- @click.option('--result-id', '-r', multiple=True, type=int,`
		`- help='Specify one or more results to be waived')`
		`+ @click.option('--subject', '-s',`
		`+ help='Specify one subject for a result to waive')`
		`+ @click.option('--testcase', '-t',`
		`+ help='Specify a testcase for the subject')`
		`@click.option('--product-version', '-p',`
		`help='Specify one of PDC\'s product version identifiers.')`
		`@click.option('--waived/--no-waived', default=True,`
		`help='Whether or not the result is waived')`
		`@click.option('--comment', '-c',`
		`help='A comment explaining why the result is waived')`
		`- def cli(comment, waived, product_version, result_id, config_file):`
		`+ def cli(comment, waived, product_version, testcase, subject, config_file):`
		`"""`
		`Creates new waivers against test results.`

		`Examples:`

		`- waiverdb-cli -r 123 -r 456 -p "fedora-26" -c "It's dead!"`
		`+ waiverdb-cli -t dist.rpmlint -s '{"item": "python-requests-1.2.3-1.fc26",`
		`+ "type": "koji_build"}'`
		`+ -p "fedora-26" -c "It's dead!"`

		`"""`
		`config = configparser.SafeConfigParser()`
		`+`
		`config.read(config_file)`
		`validate_config(config)`

		`if not product_version:`
		`raise click.ClickException('Please specify product version')`
		`- # This name makes more sense for the values of result_id, where as --result-id`
		`- # makes more sense from the cli perspective.`
		`- result_ids = result_id`
		`- if not result_ids:`
		`- raise click.ClickException('Please specify one or more result ids to waive')`
		`+ if not subject:`
		`+ raise click.ClickException('Please specify one subject')`
		`+ if not testcase:`
		`+ raise click.ClickException('Please specify testcase')`

		`auth_method = config.get('waiverdb', 'auth_method')`
		`+ data = {`
		`+ 'subject': json.loads(subject),`
		`+ 'testcase': testcase,`
		`+ 'waived': waived,`
		`+ 'product_version': product_version,`
		`+ 'comment': comment`
		`+ }`
		`+ api_url = config.get('waiverdb', 'api_url')`
		`if auth_method == 'OIDC':`
		`# Try to import this now so the user gets immediate feedback if`
		`# it isn't installed`
		`@@ -84,31 +96,13 @@`
		`{'Token': 'Token', 'Authorization': 'Authorization'},`
		`config.get('waiverdb', 'oidc_client_id'),`
		`oidc_client_secret)`
		`- for result_id in result_ids:`
		`- data = {`
		`- 'result_id': result_id,`
		`- 'waived': waived,`
		`- 'product_version': product_version,`
		`- 'comment': comment`
		`- }`
		`- api_url = config.get('waiverdb', 'api_url')`
		`- scopes = config.get('waiverdb', 'oidc_scopes').strip().splitlines()`
		`- resp = oidc.send_request(`
		`- scopes=scopes,`
		`- url='{0}/waivers/'.format(api_url.rstrip('/')),`
		`- data=json.dumps(data),`
		`- headers={'Content-Type': 'application/json'},`
		`- timeout=60)`
		`- if not resp.ok:`
		`- try:`
		`- error_msg = resp.json()['message']`
		`- except (ValueError, KeyError):`
		`- error_msg = resp.text`
		`- raise click.ClickException(`
		`- 'Failed to create waiver for result {0}:\n{1}'`
		`- .format(result_id, error_msg))`
		`- click.echo('Created waiver {0} for result {1}'.format(`
		`- resp.json()['id'], result_id))`
		`+ scopes = config.get('waiverdb', 'oidc_scopes').strip().splitlines()`
		`+ resp = oidc.send_request(`
		`+ scopes=scopes,`
		`+ url='{0}/waivers/'.format(api_url.rstrip('/')),`
		`+ data=json.dumps(data),`
		`+ headers={'Content-Type': 'application/json'},`
		`+ timeout=60)`
		`elif auth_method == 'Kerberos':`
		`# Try to import this now so the user gets immediate feedback if`
		`# it isn't installed`
		`@@ -117,31 +111,28 @@`
		`except ImportError:`
		`raise click.ClickException('python-requests-kerberos needs to be installed')`
		`auth = requests_kerberos.HTTPKerberosAuth(mutual_authentication=requests_kerberos.OPTIONAL)`
		`- for result_id in result_ids:`
		`- data = {`
		`- 'result_id': result_id,`
		`- 'waived': waived,`
		`- 'product_version': product_version,`
		`- 'comment': comment`
		`- }`
		`- api_url = config.get('waiverdb', 'api_url')`
		`- resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')),`
		`- data=json.dumps(data), auth=auth,`
		`- headers={'Content-Type': 'application/json'},`
		`- timeout=60)`
		`- if resp.status_code == 401:`
		`- raise click.ClickException('WaiverDB authentication using Kerberos failed. '`
		`- 'Make sure you have a valid Kerberos ticket.')`
		`- if not resp.ok:`
		`- try:`
		`- error_msg = resp.json()['message']`
		`- except (ValueError, KeyError):`
		`- error_msg = resp.text`
		`- raise click.ClickException(`
		`- 'Failed to create waiver for result {0}:\n{1}'`
		`- .format(result_id, error_msg))`
		`- click.echo('Created waiver {0} for result {1}'.format(`
		`- resp.json()['id'], result_id))`
		`+ resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')),`
		`+ data=json.dumps(data), auth=auth,`
		`+ headers={'Content-Type': 'application/json'},`
		`+ timeout=60)`
		`+ if resp.status_code == 401:`
		`+ raise click.ClickException('WaiverDB authentication using Kerberos failed. '`
		`+ 'Make sure you have a valid Kerberos ticket.')`
		`+ elif auth_method == 'dummy':`
		`+ resp = requests.request('POST', '{0}/waivers/'.format(api_url.rstrip('/')),`
		`+ data=json.dumps(data), auth=('user', 'pass'),`
		`+ headers={'Content-Type': 'application/json'},`
		`+ timeout=60)`
		`+ if not resp.ok:`
		`+ try:`
		`+ error_msg = resp.json()['message']`
		`+ except (ValueError, KeyError):`
		`+ error_msg = resp.text`
		`+ raise click.ClickException(`
		`+ 'Failed to create waiver for result with subject {0} and testcase {1}:\n{2}'`
		`+ .format(subject, testcase, error_msg))`
		`+ click.echo('Created waiver {0} for result with subject {1} and testcase {2}'.format(`
		`+ resp.json()['id'], subject, testcase))`


		`if __name__ == '__main__':`

waiverdb/events.py

file modified

+2 -1

		`@@ -48,7 +48,8 @@`
		`"waived": true,`
		`"timestamp": "2017-03-16T17:42:04.209638",`
		`"product_version": "Satellite 6.3",`
		`- "result_id": 1,`
		`+ "subject": "{\"a.nice.example\": \"this-is-a-really-nice-example\"}",`
		`+ "testcase": "t.e.s.t.case",`
		`"proxied_by": null,`
		`"id": 15`
		`}`

waiverdb/fields.py

file modified

+3 -1

		`@@ -2,9 +2,11 @@`

		`from flask_restful import fields`

		`+`
		`waiver_fields = {`
		`'id': fields.Integer,`
		`- 'result_id': fields.Integer,`
		`+ 'subject': fields.Raw,`
		`+ 'testcase': fields.String,`
		`'username': fields.String,`
		`'proxied_by': fields.String,`
		`'product_version': fields.String,`

waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py

file added

+37

		`@@ -0,0 +1,37 @@`
		`+ """waive absence of result`
		`+`
		`+ Revision ID: f2772c2c64a6`
		`+ Revises: 0a74cdab732a`
		`+ Create Date: 2017-12-04 10:03:54.792758`
		`+`
		`+ """`
		`+`
		`+ from alembic import op`
		`+ import sqlalchemy as sa`
		`+`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = 'f2772c2c64a6'`
		`+ down_revision = '0a74cdab732a'`
		`+`
		`+`
		`+ def upgrade():`
		`+ op.add_column('waiver', sa.Column('subject', sa.Text(), nullable=True, index=True))`
		`+ op.add_column('waiver', sa.Column('testcase', sa.Text(), nullable=True, index=True))`
		`+`
		`+ # SQLite has some problem in dropping/altering columns.`
		`+ # So in this way Alembic should do some behind the scenes`
		`+ # with: make new table - copy data - drop old table - rename new table`
		`+ with op.batch_alter_table('waiver') as batch_op:`
		`+ batch_op.alter_column('subject', nullable=False)`
		`+ batch_op.alter_column('testcase', nullable=False)`
		`+ batch_op.drop_column('result_id')`
		`+`
		`+`
		`+ def downgrade():`
		`+ op.add_column('waiver', sa.Column('result_id', sa.INTEGER(), nullable=True))`
		`+`
		`+ with op.batch_alter_table('waiver') as batch_op:`
		`+ batch_op.drop_column('testcase')`
		`+ batch_op.drop_column('subject')`
		`+ batch_op.alter_column('result_id', nullable=False)`

waiverdb/models/waivers.py

file modified

+24 -7

		`@@ -2,11 +2,14 @@`

		`import datetime`
		`from .base import db`
		`+ from sqlalchemy import or_, and_`
		`+ from sqlalchemy_utils import JSONType`


		`class Waiver(db.Model):`
		`id = db.Column(db.Integer, primary_key=True)`
		`- result_id = db.Column(db.Integer, nullable=False)`
		`+ subject = db.Column(JSONType, nullable=False, index=True)`
		`+ testcase = db.Column(db.Text, nullable=False, index=True)`
		`username = db.Column(db.String(255), nullable=False)`
		`proxied_by = db.Column(db.String(255))`
		`product_version = db.Column(db.String(200), nullable=False)`
		`@@ -14,9 +17,10 @@`
		`comment = db.Column(db.Text)`
		`timestamp = db.Column(db.DateTime, default=datetime.datetime.utcnow)`

		`- def __init__(self, result_id, username, product_version, waived=False, comment=None,`
		`- proxied_by=None):`
		`- self.result_id = result_id`
		`+ def __init__(self, subject, testcase, username, product_version, waived=False,`
		`+ comment=None, proxied_by=None):`
		`+ self.subject = subject`
		`+ self.testcase = testcase`
		`self.username = username`
		`self.product_version = product_version`
		`self.waived = waived`
		`@@ -24,6 +28,19 @@`
		`self.proxied_by = proxied_by`

		`def __repr__(self):`
		`- return '%s(result_id=%r, username=%r, product_version=%r, waived=%r)' % \`
		`- (self.__class__.__name__, self.result_id, self.username, self.product_version,`
		`- self.waived)`
		`+ return '%s(subject=%r, testcase=%r, username=%r, product_version=%r, waived=%r)' % \`
		`+ (self.__class__.__name__, self.subject, self.testcase, self.username,`
		`+ self.product_version, self.waived)`
		`+`
		`+ @classmethod`
		`+ def by_results(cls, query, results):`
		`+ return query.filter(or_(*[`
		`+ and_(`
		`+ cls.subject == d['subject'],`
		`+ cls.testcase == d['testcase']`
		`+ ) if d.get('testcase', None) else`
		`+ and_(`
		`+ cls.subject == d['subject']`
		`+ ) if d.get('subject', None) else`
		`+ and_() for d in results`
mjia commented 6 years ago So is it possible that people want to filter waivers by testcase only? As I understand, by given a result like {'testcase': 'dist.rpmdeplint'}, it seems this method would return all the results?
gnaponie commented 6 years ago I thought it could be a possible use case. I guess I talked about it with Ralph. Do you think is it not a possible use case?
mjia commented 6 years ago If we are going to change the API to optionally accept subject/testcase, this is possible.
		`+ ]))`

no initial comment

21 new commits added

Merge branch 'noresult' of ssh://pagure.io/forks/gnaponie/waiverdb into noresult
fix conflict
fix Flake8
Waive the absence of result and dummy auth for CLI
Make it so that the result_subject doesn't need to be json encoded.
add proxyuser waiving support
Adds database migration support
Returned error messages in JSON
Waive absence of a result - tmp commit
Waive absence result - still miss the test
replace 'proxy_user' with 'username'
fix some typos in the previous commit
update docs to correct the steps of setting up dev env
add ssl cert auth support
add proxyuser waiving support
Adds database migration support
fix flake8
Added more information for setup environment
Added test for 404 and 500 and json format
Fixed some errors in the doc.
Returned error messages in JSON

6 years ago

rebased onto e4fb27081d903febd0ca1fc046490804afc6d45c

6 years ago

mjia commented on line 24 of waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py 6 years ago

Hmm, what problem have you seen? I can't see it as I got an error when running this migration script.
File "/home/mjia/waiverdb/waiverdb/models/waivers.py", line 27, in <module>
MagicJSON = types.JSON().with_variant(StringyJSON, 'sqlite')
AttributeError: 'module' object has no attribute 'JSON'

mjia commented on line 21 of waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py 6 years ago

Can we use s/result_subject/subject and result_testcase/testcase? I think adding 'result_' prefix is superfluous here.

mjia commented on line 10 of waiverdb/models/waivers.py 6 years ago

Instead of carrying this code, I suggest we should use SQLAlchemy-Utils which is well maintained and developed.
http://sqlalchemy-utils.readthedocs.io/en/latest/_modules/sqlalchemy_utils/types/json.html
It is also available in koji.

https://koji.fedoraproject.org/koji/packageinfo?packageID=19060

mjia commented on line 35 of waiverdb/models/waivers.py 6 years ago

Since Greenwave or other apps would query the waivers with those two columns quite often, we should use index on them.

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

I do not think we need to log sqlalchemy in the prod. If people want to debug it, they can simply set SQLALCHEMY_ECHO = True.
http://flask-sqlalchemy.pocoo.org/2.3/config/

mjia commented on line 14 of waiverdb/fields.py 6 years ago

would fields.Raw work?
http://flask-restful.readthedocs.io/en/latest/api.html#fields.Raw

mjia commented on line 49 of waiverdb/api_v1.py 6 years ago

This needs to update to reflect that this is a list of results identified by subject and testcase.

mjia commented on line 68 of waiverdb/api_v1.py 6 years ago

We also need to consider one case that subject and testcase are all missing in the dict.

mjia commented on line 88 of waiverdb/api_v1.py 6 years ago

I think it should group by testcase as well.

mjia commented on line 166 of waiverdb/api_v1.py 6 years ago

We should use a real example here which include more than one result. The response should match this as well.

mjia commented on line 220 of waiverdb/api_v1.py 6 years ago

It would be nice to move this into a classmethod of Waiver. Something like:
@classmethod
def by_results(cls, results):
return cls.query.filter(or_(*[....]))

Then you call Waiver.by_results(results) here and above.

ralph commented 6 years ago

Relates to greenwave#111.

mprahl commented on line 61 of waiverdb/api_v1.py 6 years ago

Why is the argument named result when it's expected to be a list? I would imagine this should be named results.

mprahl commented on line 62 of waiverdb/api_v1.py 6 years ago

Optional: I'd prefer if the list was created outside of the query for readability reasons, but this is fine.

mprahl commented on line 64 of waiverdb/api_v1.py 6 years ago

Suggestion: Is it worth checking to see if subject actually has a value and that subject and testcase are indeed strings?

Edited 6 years ago by mprahl

mprahl commented on line 80 of waiverdb/api_v1.py 6 years ago

A more specific exception to catch would be nice since this could mask errors elsewhere and it would suppress a traceback that'd normally show up in the logs.

mprahl commented on line 133 of waiverdb/api_v1.py 6 years ago

Same as above. I believe you want to catch the NotFound exception.

mprahl commented on line 150 of waiverdb/api_v1.py 6 years ago

"erros" => "errors"

mprahl commented on line 211 of waiverdb/api_v1.py 6 years ago

You can just replace this with:
if data.get('results'):

sochotni commented on line 6 of waiverdb/logger.py 6 years ago

Re logging sqlalchemy - I'd just point out that accessing production logs is often problematic for devs. It's even harder if you have to instruct someone with access to the machine to run semi-random commands to get something useful from the logs.

mprahl commented on line 212 of waiverdb/api_v1.py 6 years ago

Same as above.

mprahl commented on line 230 of waiverdb/api_v1.py 6 years ago

Same as above.

mprahl commented on line 115 of waiverdb/cli.py 6 years ago

You should move the data and api_url definitions out of the if blocks since all the if blocks repeat these two lines.

mprahl commented on line 124 of waiverdb/cli.py 6 years ago

I would move this whole if block to after the execution of the other if blocks because you are repeating this same code.

mprahl commented on line 20 of waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py 6 years ago

Remove these automatic comments

ralph commented on line 14 of waiverdb/fields.py 6 years ago

Ooo, good find. That might do it. (I contributed this Unchanged thing.. let's try Raw, and if it works, lose the Unchanged field).

ralph commented on line 10 of waiverdb/models/waivers.py 6 years ago

Nice find!

ralph commented on line 35 of waiverdb/models/waivers.py 6 years ago

Good call. That will need a change both here and in the migration script.

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

But you wouldn't want to log all the SQL query outputs in production as most of the time they are useless and annoying.

gnaponie commented on line 24 of waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py 6 years ago

I had problems altering and dropping tables with SQLite: http://www.sqlite.org/lang_altertable.html --> "SQLite supports a limited subset of ALTER TABLE."

If I try to add a column with "nullable=False", but no default value I get an error (it is ok to create a table with a column like that if the table is new, but not to add a column like that). And even if in the model I specify "nullable=False", but I specify "nullable=True" in the script it will be "nullable=True" in the database (and I don't want that).

So I thought to create the table and then alter the column, but there's no complete support in SQLite for altering column. So I had to use this "op.batch_alter_table" from alembic. And it seems to work.

Does it make sense?

mjia commented on line 24 of waiverdb/migrations/versions/f2772c2c64a6_waive_absence_of_result.py 6 years ago

Thanks, I see.

1 new commit added

Fixed all the reported problems and added some other tests

6 years ago

gnaponie commented 6 years ago

I've posted a new commit. It should be fine now.
Thanks

dcallagh commented 6 years ago

Ah, it looks like you've addressed the comments by just adding a new commit on top of the existing ones... I think that makes it a bit harder to review the whole thing, and it leaves a bit of a mess in the git history.

It would be cleaner if you could rebase the series and fix the issues in each commit where they are introduced. So that it keeps a clean, logical sequence of changes, rather than changing something in one commit and changing it again in a subsequent commit.

Also, it looks like you might have already rebased at one point and accidentally squashed some commits together? At least, when I try to read the series as is, some things don't seem to make sense to me. For example, https://pagure.io/fork/gnaponie/waiverdb/c/d8f5e2644552a83c47b1bcb6d1a51807f30ce124 says that it is "Waive the absence of result and dummy auth for CLI" but the actual changes appear to be just some unrelated tidying up of the code.

gnaponie commented 6 years ago

Ah, it looks like you've addressed the comments by just adding a new commit on top of the existing ones... I think that makes it a bit harder to review the whole thing, and it leaves a bit of a mess in the git history.
It would be cleaner if you could rebase the series and fix the issues in each commit where they are introduced. So that it keeps a clean, logical sequence of changes, rather than changing something in one commit and changing it again in a subsequent commit.

I'll try to clean it a little...

Also, it looks like you might have already rebased at one point and accidentally squashed some commits together? At least, when I try to read the series as is, some things don't seem to make sense to me. For example, https://pagure.io/fork/gnaponie/waiverdb/c/d8f5e2644552a83c47b1bcb6d1a51807f30ce124 says that it is "Waive the absence of result and dummy auth for CLI" but the actual changes appear to be just some unrelated tidying up of the code.

Yeah, because I had a lots of commits... so in the end I've just rebased almost everything, but maybe the comments weren't so good

Edited 6 years ago by gnaponie

rebased onto 0f6f87de9075f667542be0e455b00f5ce291472c

6 years ago

gnaponie commented 6 years ago

@sochotni helped me to reorganize the commits.
@dcallagh do you think it is fine now?

mjia commented on line 64 of waiverdb/api_v1.py 6 years ago

You do not need to have two ifs here as you can just do:
if not isinstance(d.get('subject', None), dict):

mjia commented on line 68 of waiverdb/api_v1.py 6 years ago

Same as above.

mjia commented on line 70 of waiverdb/api_v1.py 6 years ago

Maybe we should give a clear message like "testcase is missing in 'results' parameter"

mjia commented on line 234 of waiverdb/api_v1.py 6 years ago

Same as above, you can just remove this line.

mjia commented on line 238 of waiverdb/api_v1.py 6 years ago

Same as this one.

mjia commented on line 45 of waiverdb/cli.py 6 years ago

This breaks backward compatibility as right now we are supporting for waiving multiple results. On the other hand, you could use tuples http://click.pocoo.org/5/options/#tuples-as-multi-value-options

And then you could parse the data as something like this:
for s in subjects:
data['subject'] = dict(s)

Note: we may need to check the given subject can be converted to a dict.

So to create a waiver, a command would be:

waiverdb-cli -t dist.rpmlint -s item=python-requests-1.2.3-1.fc26 type=koji_build -p "fedora-26" -c "It's dead!"

mjia commented on line 52 of waiverdb/models/waivers.py 6 years ago

So is it possible that people want to filter waivers by testcase only? As I understand, by given a result like {'testcase': 'dist.rpmdeplint'}, it seems this method would return all the results?

mjia commented 6 years ago

@gnaponie , since the current Jenkins set up only runs after the PR is merged, you could create a job like the one I'm using(I will send you a link via email as we are using an internal redhat jenkins) and manully run it against your local branch. So this will you tell you at the early stage about whether your patch would affect other things before merging.

gnaponie commented on line 45 of waiverdb/cli.py 6 years ago

@mjia maybe I didn't understand your proposal... but is this way, how can the user specify different testcases for different subjects? How can you know which testcase is related to the subject?

We can consider the position, but maybe is a little tricky for the user to use it...
Example:
waiverdb-cli -t dist.rpmlint -s subject1 -t testcase1 -s subject2 -t testcase2 -p "fedora-26" -c "It's dead!"
The subject1 is associated to the testcase1 and the subject2 is associated to the testcase2.
Does it make sense?

gnaponie commented on line 52 of waiverdb/models/waivers.py 6 years ago

I thought it could be a possible use case.
I guess I talked about it with Ralph.
Do you think is it not a possible use case?

gnaponie commented on line 64 of waiverdb/api_v1.py 6 years ago

mmm I think at this point there was some confusion (in my head at least...). You can filter without a subject... So if the user makes a request without specifying a subject (for example), this:
not isinstance(d.get('subject', None), dict)

would be -->
d.get('subject', None) will be None
not isinstance(d.get('subject', None), dict) will be True

so you will have a Bad request... even if you can filter without a subject (or testcase, or both).
Because I think the error message (for the bad request) should say to the user that he used the wrong format... and not that the parameter is missing.

Does it make sense?

dcallagh commented on line 49 of waiverdb/api_v1.py 6 years ago

This API doesn't make sense to me... not sure if I am just behind the times. But I thought that we changed it so that a waiver is recorded for a subject and testcase, right?

So if this is for selecting out waivers, and optionally filtering them, then surely it should just be two parameters (both optional), 'subject' and 'testcase' which filter the waivers down the requested value if the parameter is present.

This thing where it has to be a JSON-encoded array of dicts with 'subject' and 'testcase' seems very complex and difficult for the callers to use.

Who are the callers anyway?

mjia commented on line 49 of waiverdb/api_v1.py 6 years ago

Good point. As far as I know, no caller is calling this right now.

mjia commented on line 45 of waiverdb/cli.py 6 years ago

Okay, I did not realize that we would accept multiple testcases as well. So I guess we could use http://click.pocoo.org/5/options/#tuples-as-multi-value-options. An example would be
@click.option('--result', '-r', type=(unicode, unicode), multiple=True)
...
Then the caller could use a command like this to create multiple waivers

waiverdb-cli -r subject1 testcase1 -r subject2 testcase2 -p "fedora-26" -c "It's dead!"

And we need to clearly document that each result is in the form of 'subject testcase'.

Edited 6 years ago by mjia

mjia commented on line 64 of waiverdb/api_v1.py 6 years ago

It seems like we are going to change the API so I guess this part wouldn't be needed anymore.

mjia commented on line 52 of waiverdb/models/waivers.py 6 years ago

If we are going to change the API to optionally accept subject/testcase, this is possible.

gnaponie commented on line 45 of waiverdb/cli.py 6 years ago

But still... a subject could be a complex data (a dictionary), and with Ralph we assumed that the user will provide the dictionary in this way (as example):
-s item=python-requests-1.2.3-1.fc26 type=koji_build
(and we split on the space and then on the '=').

It is for sure possible to add a testcase as you said, but it will be a little tricky for the user I guess... BTW I'll talk about it with Dan when he will be here in Brno, we will try to find a solution for this and we will write it here.

mjia commented on line 45 of waiverdb/cli.py 6 years ago

That's okay as you can have a callback for validation to make sure the result is in the form of 'subject' and 'testcase'.
http://click.pocoo.org/5/options/#callbacks-for-validation

dcallagh commented 6 years ago

Okay, had a quick chat with Giulia about this. I now understand the whole thing a lot better (that's my bad for not keeping up with all the discussions that were going on).

We think that, for the GET /waivers/ endpoint, we can simplify that to just accept optionally one testcase, and optionally one subject as a JSON-encoded dict. That will keep things simple. There are no actual known users of that API either, it is really just provided for completeness.

The API which Greenwave will use is the POST /waivers/+by-subjects-and-test-cases. It takes the more complicated JSON structure as the body of the request, which seems fine.

And I also suggested for the CLI for submitting waivers, to keep things simple we should just accept a JSON-encoded dict for the subject, rather than inventing some new 'key=value key=value' type of syntax. I personally think that having a space-delimited format (with spaces inside a single shell word) or an argument that accepts multiple words is difficult for people to get right. Typing out JSON like -s '{"item": ...}' is not very user friendly but at least the syntax is quite unambiguous. And hopefully users will not need to worry about the CLI anyway, we will have Bodhi waiving instead.

rebased onto 3662ec2e90dcb2925549af48d48f9c4f0a4f360d

6 years ago

gnaponie commented 6 years ago

I made a new commit: 68e2b2e

ralph commented 6 years ago

Rebased and merged. Thank you!