PR#510: Proceed other rules when there's no source URL in Koji build - greenwave

greenwave

#510 Proceed other rules when there's no source URL in Koji build

Merged 4 years ago by vmaljulin. Opened 4 years ago by vmaljulin.

vmaljulin/greenwave FACTORY-4685 into master

Proceed other rules when there's no source URL in Koji build

Valerij Maljulin • 4 years ago

ed6998a

greenwave/policies.py

file modified

+8 -2

		`@@ -395,8 +395,14 @@`
		`if policy.subject_type not in ['koji_build', 'redhat-module', 'redhat-container-image']:`
		`return []`

		`- pkg_namespace, pkg_name, rev = greenwave.resources.retrieve_scm_from_koji(`
		`- subject_identifier)`
		`+ try:`
		`+ pkg_namespace, pkg_name, rev = greenwave.resources.retrieve_scm_from_koji(`
		`+ subject_identifier`
		`+ )`
		`+ except greenwave.resources.NoSourceException as e:`
		`+ log.error(e)`
		`+ return None`
		`+`
		`# if the element is actually a container and not a pkg there will be a "-container"`
		`# string at the end of the "pkg_name" and it will not match with the one in the`
		`# gating.yaml URL`

greenwave/resources.py

file modified

+5 -1

		`@@ -123,6 +123,10 @@`
		`**request_args)`


		`+ class NoSourceException(RuntimeError):`
		`+ pass`
		`+`
		`+`
		`@cached`
		`def retrieve_scm_from_koji(nvr):`
		`""" Retrieve cached rev and namespace from koji using the nvr """`
		`@@ -141,7 +145,7 @@`

		`source = build.get('source')`
		`if not source:`
		`- raise BadGateway(`
		`+ raise NoSourceException(`
		`'Failed to retrieve SCM URL from Koji build "{}" at "{}" '`
		`'(expected SCM URL in "source" attribute)'`
		`.format(nvr, koji_url))`

greenwave/tests/test_policies.py

file modified

+47

		`@@ -1022,3 +1022,50 @@`
		`decision = policy.check('fedora-26', nvr, results)`
		`assert len(decision) == 1`
		`assert isinstance(decision[0], TestResultFailed)`
		`+`
		`+`
		`+ @pytest.mark.parametrize('two_rules', (True, False))`
		`+ def test_on_demand_policy_match(two_rules):`
		`+ """ Testing the RemoteRule with the koji interaction when on_demand policy is given.`
		`+ In this case we are just mocking koji """`
		`+`
		`+ nvr = 'httpd-2.4.el9000'`
		`+`
		`+ serverside_json = {`
		`+ 'product_version': 'fedora-30',`
		`+ 'id': 'taskotron_release_critical_tasks_with_remoterule',`
		`+ 'subject_type': 'koji_build',`
		`+ 'subject_identifier': nvr,`
		`+ 'rules': [`
		`+ {`
		`+ 'type': 'RemoteRule'`
		`+ }`
		`+ ],`
		`+ }`
		`+`
		`+ if two_rules:`
		`+ serverside_json['rules'].append({`
		`+ "type": "PassingTestCaseRule",`
		`+ "test_case_name": "fake.testcase.tier0.validation"`
		`+ })`
		`+`
		`+ app = create_app('greenwave.config.TestingConfig')`
		`+ with app.app_context():`
		`+ with mock.patch('xmlrpc.client.ServerProxy') as koji_server:`
		`+ koji_server_instance = mock.MagicMock()`
		`+ koji_server_instance.getBuild.return_value = {'source': None}`
		`+ koji_server.return_value = koji_server_instance`
		`+ policy = OnDemandPolicy.create_from_json(serverside_json)`
		`+`
		`+ rv = policy.matches(subject_identifier=nvr)`
		`+`
		`+ koji_server_instance.getBuild.assert_called_once()`
		`+ assert rv is two_rules`
		`+`
		`+ results = DummyResultsRetriever(`
		`+ nvr, 'fake.testcase.tier0.validation', 'PASSED', 'koji_build'`
		`+ )`
		`+ decision = policy.check('fedora-30', nvr, results)`
		`+ if two_rules:`
		`+ assert len(decision) == 1`
		`+ assert isinstance(decision[0], RuleSatisfied)`

greenwave/tests/test_retrieve_gating_yaml.py

file modified

+4 -2

		`@@ -9,7 +9,9 @@`

		`import greenwave.app_factory`
		`from greenwave.resources import (`
		`- retrieve_scm_from_koji_build, retrieve_yaml_remote_rule, retrieve_scm_from_koji)`
		`+ retrieve_scm_from_koji_build, retrieve_yaml_remote_rule, retrieve_scm_from_koji,`
		`+ NoSourceException`
		`+ )`

		`KOJI_URL = 'https://koji.fedoraproject.org/kojihub'`

		`@@ -52,7 +54,7 @@`
		`'nvr': nvr`
		`}`
		`expected_error = 'expected SCM URL in "source" attribute'`
		`- with pytest.raises(BadGateway, match=expected_error):`
		`+ with pytest.raises(NoSourceException, match=expected_error):`
		`retrieve_scm_from_koji_build(nvr, build, KOJI_URL)`

vmaljulin commented 4 years ago

Signed-off-by: Valerij Maljulin vmaljuli@redhat.com

vmaljulin commented 4 years ago

DO NOT MERGE YET!

gnaponie commented 4 years ago

Can you add a test with this scenario:
you have 2 rules in a policy in the global conf: PassingTestCaseRule and RemoteRule. You can create a result that would make the PassingTestCaseRule pass. Then you can mock the failure (NoSourceException) and check if the "PassingTestCaseRule" was processed (satisfied) or ignored.

This test would check if the behaviour is the expected one.

lholecek commented on line 12 of greenwave/policies.py 4 years ago

No need to log the stacktrace, the error message would be enough.

rebased onto 7ec80e3abb7089592a3a7096403819c961c88456

4 years ago

vmaljulin commented 4 years ago

Comments were addressed

lholecek commented on line 43 of greenwave/tests/test_policies.py 4 years ago

Can you rather mock just the koji data? (I.e. mock xmlrpc.client.ServerProxy.getBuild().)

rebased onto ab5ce3c020e5da4b71622492984c4eb208e3882d

4 years ago

vmaljulin commented 4 years ago

@lholecek Done

lholecek commented on line 38 of greenwave/tests/test_policies.py 4 years ago

I think you can do just koji_server_instance.getBuild.return_value = {'source': None} for clarity.

lholecek commented 4 years ago

rebased onto 30b5e9781081c4919e105600b1791d81546dbab4

4 years ago

gnaponie commented 4 years ago

Can you continue the test with the creation of a result and then check if the decision is satisfied?
This would happen only when "two_rules" is True.

Edited 4 years ago by gnaponie

gnaponie commented 4 years ago

PS. note that this behaviour is not strictly related to the "on-demand" feature, but applies to all cases where the RemoteRule is applicable. So it's easier you can test it also with the "normal" behaviour.

rebased onto 1ece51263c58939f89eccef87865f79694f3cc30

4 years ago

lholecek commented 4 years ago

gnaponie commented on line 47 of greenwave/tests/test_policies.py 4 years ago

You can avoid of using "nsvc" here, you can use always "nvr" and avoid declaring "nsvc" above.
And then in:
decision = policy.check('fedora-29', nsvc, results)
You would use "nvr" too and instead of "fedora-29" you can use "fedora-30".

I'm actually surprised that it succeeds.. It's a bit strange because you don't have a policy for fedora-29...

rebased onto ed6998a

4 years ago

gnaponie commented 4 years ago

Thank you for applying the changes! It looks good.
+1

Pull-Request has been merged by vmaljulin

4 years ago