#99 Waive the absence of a result and dummy auth for cli
Closed 2 years ago by ralph. Opened 2 years ago by gnaponie.
gnaponie/waiverdb noresult  into  master

file modified
+1
@@ -11,6 +11,7 @@ 

  kerberos >= 1.1.1

  flask-oidc

  systemd

+ sqlalchemy-utils

  # packages for the unit tests

  pytest >= 2.4.2

  mock

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):

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',

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

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',

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

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

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 2 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 2 years ago

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

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 2 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 2 years ago

Same as above.

+                     if not isinstance(d.get('testcase', None), basestring):

+                         raise BadRequest("'results' parameter should be a list \
mjia commented 2 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 2 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 2 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')

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__':

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

            }

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,

@@ -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)

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 2 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?

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 2 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
2 years ago

rebased onto e4fb27081d903febd0ca1fc046490804afc6d45c

2 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'

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

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

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

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/

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

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

I think it should group by testcase as well.

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

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.

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

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

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

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.

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

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

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.

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

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

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

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

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

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?

1 new commit added

  • Fixed all the reported problems and added some other tests
2 years ago

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

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.

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

rebased onto 0f6f87de9075f667542be0e455b00f5ce291472c

2 years ago

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

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

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

Same as above, you can just remove this line.

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!"

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 , 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.

@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?

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?

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?

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?

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

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'.

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

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

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.

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

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

2 years ago

I made a new commit: 68e2b2e

Rebased and merged. Thank you!

Pull-Request has been closed by ralph

2 years ago

Build 68e2b2e778e962d4caceabd36432cd75ff46dea9 FAILED!
Rebase or make new commits to rebuild.

Build 68e2b2e778e962d4caceabd36432cd75ff46dea9 FAILED!
Rebase or make new commits to rebuild.