PR#457: Add missing-gating-yaml answer - greenwave

greenwave

#457 Add missing-gating-yaml answer

Closed 4 years ago by lholecek. Opened 4 years ago by lholecek.

lholecek/greenwave missing-gating-yaml-answer into master

Lukas Holecek • 4 years ago

09474c0

file modified

+27

		`@@ -69,6 +69,33 @@`
		`The side effect is that all the policies defined in the gating.yaml`
		`file will be completely ignored by Greenwave.`

		`+ .. _missing-gating-yaml:`
		`+`
		`+`
		`+ Missing gating.yaml file`
		`+ ------------------------`
		`+`
		`+ If gating.yaml file is missing (i.e. not present in dist-git repo of the tested`
lucarval commented 4 years ago Can we document that if the file exists but it's empty, it'll not qualify as `missing-gating-yaml` ?
		`+ package in the required revision), "missing-gating-yaml" will appear in`
		`+ satisfied requirements.`
		`+`
		`+ .. code-block:: json`
		`+`
		`+ {`
		`+ "applicable_policies": ["some_policy"],`
		`+ "policies_satisfied": true,`
		`+ "satisfied_requirements": [{`
		`+ "subject_identifier": "nethack-1.2.3-1.f31",`
		`+ "subject_type": "koji_build",`
		`+ "type": "missing-gating-yaml"`
gnaponie commented 4 years ago That is a bit different from how I pictured it... I was thinking more: you get the same response you will get now, but with an additional key like: "missing-gating-yaml": True Or maybe something like: "missing-gating-yaml": [list of matching policies ids that have a remote rule that has no gating.yaml] Or maybe instead of that list we might put the list of the NVRs for which we failed to retrieve the gating.yaml. I don't think it is correct to put "missing-gating-yaml" as "type". The gating.yaml can be missing, but this doesn't mean that the policies are not satisfied. We agreed in the past that we shouldn't block the other policies if the remote policies cannot be found, and I think this is still correct. I would expect complains if we change this behavior. I think this PR shouldn't change the behavior. Just add a field in the response. Edited 4 years ago by gnaponie
		`+ }],`
		`+ "summary": "no tests are required",`
		`+ "unsatisfied_requirements": []`
		`+ }`
		`+`
		`+ This still results in the decision summary "no tests are required" if no other`
		`+ test are required.`
		`+`

		`.. _tutorial-configure-remoterule:`

greenwave/api_v1.py

file modified

+2 -1

		`@@ -471,7 +471,8 @@`
		`answers = waive_answers(answers, waivers)`

		`response = {`
		`- 'policies_satisfied': all(answer.is_satisfied for answer in answers),`
		`+ 'policies_satisfied':`
		`+ all(answer.is_satisfied or not answer.is_required for answer in answers),`
		`'summary': summarize_answers(answers),`
		`'satisfied_requirements':`
		`[answer.to_json() for answer in answers if answer.is_satisfied],`

greenwave/policies.py

file modified

+31 -2

		`@@ -62,6 +62,8 @@`
		`a subclass, depending on what the answer was.`
		`"""`

		`+ is_required = True`
		`+`
		`def to_json(self):`
		`"""`
		`Returns a machine-readable description of the problem for API responses.`
		`@@ -204,6 +206,25 @@`
		`return None`


		`+ class MissingGatingYaml(RuleSatisfied):`
		`+ """`
		`+ Remote policy not found in remote repository.`
		`+ """`
		`+`
		`+ is_required = False`
		`+`
		`+ def __init__(self, subject_type, subject_identifier):`
		`+ self.subject_type = subject_type`
		`+ self.subject_identifier = subject_identifier`
		`+`
		`+ def to_json(self):`
		`+ return {`
		`+ 'type': 'missing-gating-yaml',`
		`+ 'subject_type': self.subject_type,`
		`+ 'subject_identifier': self.subject_identifier,`
		`+ }`
		`+`
		`+`
		`class TestResultPassed(RuleSatisfied):`
		`"""`
		`A required test case passed (that is, its outcome in ResultsDB was`
		`@@ -259,7 +280,7 @@`
		`Returns:`
		`str: Human-readable summary.`
		`"""`
		`- if not answers:`
		`+ if all(not answer.is_required for answer in answers):`
		`return 'no tests are required'`

		`failure_count = len([answer for answer in answers if isinstance(answer, RuleNotSatisfied)])`
		`@@ -380,7 +401,7 @@`

		`if response is None:`
		`# greenwave extension file not found`
		`- return []`
		`+ return None`

		`policies = RemotePolicy.safe_load_all(response)`
		`if isinstance(policy, OnDemandPolicy):`
		`@@ -408,6 +429,11 @@`
		`policy.subject_type, subject_identifier, 'invalid-gating-yaml', str(e))`
		`]`

		`+ if policies is None:`
		`+ return [`
		`+ MissingGatingYaml(policy.subject_type, subject_identifier)`
		`+ ]`
		`+`
		`answers = []`
		`for remote_policy in policies:`
		`if remote_policy.matches_product_version(product_version):`
		`@@ -437,6 +463,9 @@`
		`logging.exception(`
		`'Failed to retrieve policies for %r', subject_identifier)`

		`+ if sub_policies is None:`
yashn commented 4 years ago Optional: `if not sub_policies`
		`+ return True`
		`+`
		`return any(sub_policy.matches(**attributes) for sub_policy in sub_policies)`

		`def to_json(self):`

greenwave/tests/test_api_v1.py

file modified

+43 -2

		`@@ -53,9 +53,9 @@`
		`yield mocked`


		`- def make_decision(**kwargs):`
		`+ def make_decision(policies=DEFAULT_DECISION_POLICIES, **kwargs):`
		`app = create_app('greenwave.config.TestingConfig')`
		`- app.config['policies'] = Policy.safe_load_all(dedent(DEFAULT_DECISION_POLICIES))`
		`+ app.config['policies'] = Policy.safe_load_all(dedent(policies))`
		`client = app.test_client()`
		`data = DEFAULT_DECISION_DATA.copy()`
		`data.update(kwargs)`
		`@@ -98,6 +98,47 @@`
		`mock_waivers.assert_called_once()`


		`+ def test_make_decision_with_no_tests_required(mock_results, mock_waivers):`
		`+ mock_results.return_value = []`
		`+ mock_waivers.return_value = []`
		`+ policies = """`
		`+ --- !Policy`
		`+ id: "test_policy"`
		`+ product_versions:`
		`+ - fedora-rawhide`
		`+ decision_context: test_policies`
		`+ subject_type: koji_build`
		`+ rules: []`
		`+ """`
		`+ response = make_decision(policies=policies)`
		`+ assert 200 == response.status_code`
		`+ assert 'no tests are required' == response.json['summary']`
		`+ mock_waivers.assert_not_called()`
		`+`
		`+`
		`+ def test_make_decision_missing_gating_yaml(mock_results, mock_waivers):`
		`+ mock_results.return_value = []`
		`+ mock_waivers.return_value = []`
		`+ policies = """`
		`+ --- !Policy`
		`+ id: "test_policy"`
		`+ product_versions:`
		`+ - fedora-rawhide`
		`+ decision_context: test_policies`
		`+ subject_type: koji_build`
		`+ rules:`
		`+ - !RemoteRule {}`
		`+ """`
		`+ with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:`
		`+ scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')`
		`+ with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:`
		`+ f.return_value = None`
		`+ response = make_decision(policies=policies)`
		`+ assert 200 == response.status_code`
		`+ assert 'no tests are required' == response.json['summary']`
		`+ mock_waivers.assert_not_called()`
		`+`
		`+`
		`def test_life_decision():`
		`app = create_app('greenwave.config.TestingConfig')`
		`client = app.test_client()`

greenwave/tests/test_policies.py

file modified

+28

		`@@ -10,6 +10,7 @@`
		`from greenwave.policies import (`
		`load_policies,`
		`summarize_answers,`
		`+ MissingGatingYaml,`
		`Policy,`
		`RemotePolicy,`
		`RuleSatisfied,`
		`@@ -552,6 +553,33 @@`
		`assert len(decision) == 0`


		`+ def test_remote_rule_missing_yaml():`
		`+ """ Testing the RemoteRule with a missing gating.yaml file """`
		`+ nvr = 'nethack-1.2.3-1.el9000'`
		`+ app = create_app('greenwave.config.TestingConfig')`
		`+ with app.app_context():`
		`+ with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm:`
		`+ scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb')`
		`+ with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f:`
		`+ f.return_value = None`
		`+ policies = Policy.safe_load_all(dedent("""`
		`+ --- !Policy`
		`+ id: test`
		`+ product_versions: [fedora-rawhide]`
		`+ decision_context: test`
		`+ subject_type: koji_build`
		`+ rules:`
		`+ - !RemoteRule {}`
		`+ """))`
		`+ policy = policies[0]`
		`+ results = DummyResultsRetriever()`
		`+ decision = policy.check('fedora-rawhide', nvr, results)`
		`+ assert len(decision) == 1`
		`+ assert isinstance(decision[0], MissingGatingYaml)`
		`+ assert decision[0].is_satisfied`
		`+ assert decision[0].subject_identifier == nvr`
		`+`
		`+`
		`def test_parse_policies_missing_tag():`
		`expected_error = "Missing !Policy tag"`
		`with pytest.raises(SafeYAMLError, match=expected_error):`

lholecek commented 4 years ago

Added test to check "no tests are required" summary (e.g. checked by
Bodhi).

Signed-off-by: Lukas Holecek hluk@email.cz

yashn commented on line 70 of greenwave/policies.py 4 years ago

Optional: if not sub_policies

lholecek commented 4 years ago

Optional: if not sub_policies

With None value, I'm indicating that the gating.yaml is missing. Not sure if the result of parsing the file can be empty policy list.

lucarval commented on line 11 of docs/package-specific-policies.rst 4 years ago

Can we document that if the file exists but it's empty, it'll not qualify as missing-gating-yaml ?

lucarval commented 4 years ago

@lholecek, if a RemotePolicy is required but it's gating.yml is missing, will that requirement be skipped with this change? I don't think it will, just double checking :smile:

lholecek commented 4 years ago

@lholecek, if a RemotePolicy is required but it's gating.yml is missing, will that requirement be skipped with this change? I don't think it will, just double checking 😄

missing-gating-yaml will be listed in satisfied_requirements (if RemotePolicy is required but gating.yml is missing).

Could that be an issue?

Hmm, currently, the code is confusing. The new MissingGatingYaml answer has is_required = False but it's still listed in satisfied_requirements. (The is_required is needed to have summary with no tests are required even if there are only missing gating.yaml files.)

Maybe list those answers in a new response field instead?

lholecek commented 4 years ago

Can we document that if the file exists but it's empty, it'll not qualify as missing-gating-yaml ?

I think it's currently clear from the doc (added emphasis):

If gating.yaml file is missing (i.e. not present in dist-git repo of the tested
package in the required revision), "missing-gating-yaml" will appear in
satisfied requirements.

lucarval commented 4 years ago

@lholecek, if a RemotePolicy is required but it's gating.yml is missing, will that requirement be skipped with this change? I don't think it will, just double checking 😄

missing-gating-yaml will be listed in satisfied_requirements (if RemotePolicy is required but gating.yml is missing).
Could that be an issue?
Hmm, currently, the code is confusing. The new MissingGatingYaml answer has is_required = False but it's still listed in satisfied_requirements. (The is_required is needed to have summary with no tests are required even if there are only missing gating.yaml files.)
Maybe list those answers in a new response field instead?
That sounds reasonable to me. @gnaponie, thoughts?

gnaponie commented on line 23 of docs/package-specific-policies.rst 4 years ago

That is a bit different from how I pictured it... I was thinking more: you get the same response you will get now, but with an additional key like:
"missing-gating-yaml": True

Or maybe something like:
"missing-gating-yaml": [list of matching policies ids that have a remote rule that has no gating.yaml]

Or maybe instead of that list we might put the list of the NVRs for which we failed to retrieve the gating.yaml.

I don't think it is correct to put "missing-gating-yaml" as "type". The gating.yaml can be missing, but this doesn't mean that the policies are not satisfied. We agreed in the past that we shouldn't block the other policies if the remote policies cannot be found, and I think this is still correct. I would expect complains if we change this behavior.

I think this PR shouldn't change the behavior. Just add a field in the response.

Edited 4 years ago by gnaponie

lholecek commented 4 years ago

I'm closing this because we want to do this differently but I don't like adding missing-gating-yaml to the response either - it could be overly complicated (rules will need to modify fields in response?) and solves very rare cases (remote rule on-boarding).

Pull-Request has been closed by lholecek

4 years ago