PR#167: Comment required for submitting waivers - waiverdb

waiverdb

#167 Comment required for submitting waivers

Merged 6 years ago by gnaponie. Opened 6 years ago by gnaponie.

gnaponie/waiverdb comment-required into master

Comment required for submitting waivers

Giulia Naponiello • 6 years ago

ecfd77a

tests/test_api_v10.py

file modified

+24

		`@@ -87,6 +87,30 @@`
		`assert res_data['comment'] == 'it broke'`


		`+ @patch('waiverdb.api_v1.get_resultsdb_result')`
		`+ @patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`+ def test_create_waiver_without_comment(mocked_get_user, mocked_resultsdb, client,`
		`+ session):`
		`+ mocked_resultsdb.return_value = {`
		`+ 'data': {`
		`+ 'original_spec_nvr': ['somedata'],`
		`+ },`
		`+ 'testcase': {'name': 'sometest'}`
		`+ }`
		`+`
		`+ data = {`
		`+ 'result_id': 123,`
		`+ 'product_version': 'fool-1',`
		`+ 'waived': True,`
		`+ }`
		`+ 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`
		`+ res_data = json.loads(r.get_data(as_text=True))`
		`+ assert res_data['message'] == 'Comment is a required argument.'`
		`+`
		`+`
		`@patch('waiverdb.api_v1.get_resultsdb_result', side_effect=HTTPError(response=Mock(status=404)))`
		`@patch('waiverdb.auth.get_user', return_value=('foo', {}))`
		`def test_create_waiver_with_unknown_result_id(mocked_get_user, mocked_resultsdb, client, session):`

tests/test_cli.py

file modified

+21 -2

		`@@ -123,7 +123,7 @@`
		`resultsdb_api_url=http://localhost:5001/api/v2.0`
		`""")`
		`runner = CliRunner()`
		`- args = ['-C', p.strpath, '-p', 'fedora-26']`
		`+ args = ['-C', p.strpath, '-p', 'fedora-26', '-c', 'comment']`
		`result = runner.invoke(waiverdb_cli, args)`
		`assert result.exit_code == 1`
		`assert result.output == 'Error: Please specify one subject\n'`
		`@@ -142,12 +142,31 @@`
		`resultsdb_api_url=http://localhost:5001/api/v2.0`
		`""")`
		`runner = CliRunner()`
		`- args = ['-C', p.strpath, '-p', 'fedora-26', '-s', 'subject']`
		`+ args = ['-C', p.strpath, '-p', 'fedora-26', '-s', 'subject', '-c', 'comment']`
		`result = runner.invoke(waiverdb_cli, args)`
		`assert result.exit_code == 1`
		`assert result.output == 'Error: Please specify testcase\n'`


		`+ def test_no_comment(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`
		`+ resultsdb_api_url=http://localhost:5001/api/v2.0`
		`+ """)`
		`+ runner = CliRunner()`
		`+ args = ['-C', p.strpath, '-p', 'fedora-26', '-s', 'subject', '-t', 'testcase']`
		`+ result = runner.invoke(waiverdb_cli, args)`
		`+ assert result.exit_code == 1`
		`+ assert result.output == 'Error: Please specify comment\n'`
		`+`
		`+`
		`def test_oidc_auth_is_enabled(tmpdir):`
		`# Skip if waiverdb is rebuilt for an environment where GSSAPI`
		`# authentication is used and python-openidc-client is not available.`

waiverdb/api_v1.py

file modified

		`@@ -250,6 +250,8 @@`
		`if not args['subject'] or not args['testcase']:`
		`raise BadRequest('Either result_id or subject/testcase '`
		`'are required arguments.')`
		`+ if not args['comment']:`
		`+ raise BadRequest('Comment is a required argument.')`

		`waiver = Waiver(args['subject'], args['testcase'], user,`
		`args['product_version'], args['waived'], args['comment'], proxied_by)`

waiverdb/cli.py

file modified

		`@@ -92,6 +92,8 @@`
		`result_ids = result_id`
		`if not product_version:`
		`raise click.ClickException('Please specify product version')`
		`+ if not comment:`
		`+ raise click.ClickException('Please specify comment')`
		`if result_ids and (subject or testcase):`
		`raise click.ClickException('Please specify result_id or subject/testcase. Not both')`
		`if not result_ids and not subject:`

gnaponie commented 6 years ago

The comment parameter is now required when you submit a new waiver.
Both for API and CLI. Provided some test.

lholecek commented 6 years ago

What about API (and CLI) being version-compatible? This can break existing scripts.

ralph commented 6 years ago

It's true, but I'm willing to break any such scripts in this case. It is reasonable to enforce a comment requirement.

Edited 6 years ago by ralph

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

This should be 'Comment is a required argument.'.

ralph commented 6 years ago

:+1: to the gist from me. One comment about about the text in the error response.

rebased onto 4ae23bba4d5f86146158ccddf0c9040c29028bd4

6 years ago

rebased onto 5bdad0e52853bcb8092082c0fb2b39be013756af

6 years ago

gnaponie commented 6 years ago

The comment should be ok now.
Let's wait also for @dcallagh opinion.

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

s/arguments/argument/

dcallagh commented 6 years ago

Yeah, in all our examples we have always shown it with --comment, I think because we always intended from the start that the comment should be mandatory. So I think the risk of breaking anyone here is low.

It might be nice if we could document somewhere why it's required. Maybe in the man page for the CLI that doesn't exist :-) I always hate when some tool tells me "you must supply this" but it's not obvious why it is making me do it.

lholecek commented 6 years ago

It might be nice if we could document somewhere why it's required. Maybe in the man page for the CLI that doesn't exist :-) I always hate when some tool tells me "you must supply this" but it's not obvious why it is making me do it.

I like how --help shows "This is fine" as an example comment :) ... but it's still not obvious that the comment is required parameter (though it shouldn't be required for the new --get argument - #159).

gnaponie commented 6 years ago

Would it be ok to put that the parameter is required in the help and then add in the README file an explanation of why it's required? The same for all the other required parameters...

dcallagh commented 6 years ago

Hmm yeah there are lots of things we could improve about the docs and --help output. I think let's merge this as is, and I will put up something separately for docs.

dcallagh commented 6 years ago

Oh just the typo in api_v1.py needs fixing though.

rebased onto ecfd77a

6 years ago

gnaponie commented 6 years ago

It should be ok now.

lholecek commented 6 years ago