#618 New rule type EmptyRule and new result called NoTestsRequired
Closed 3 years ago by vmaljulin. Opened 3 years ago by vmaljulin.
vmaljulin/greenwave RHELWF-2189  into  master

file modified
+8
@@ -184,6 +184,14 @@ 

  

  .. _remote-rule:

  

+ EmptyRule

+ ---------

+ 

+    This rule type is similar to previous with the only exception, that it

+    is always evaluated positively and result is ``no-tests-required`` when

+    other options were matched.

+ 

+ 

  RemoteRule

  ----------

  

file modified
+42
@@ -285,6 +285,21 @@ 

          }

  

  

+ class NoTestsRequired(RuleSatisfied):

+     """

+     Returned for EmptyRule

+     """

+     def __init__(self, subject):

+         self.subject = subject

+ 

+     def to_json(self):

+         return {

+             'type': 'no-tests-required',

+             'subject_type': self.subject.type,

+             'subject_identifier': self.subject.identifier,

+         }

+ 

+ 

  class BlacklistedInPolicy(RuleSatisfied):

      """

      Package was blacklisted in policy.
@@ -652,6 +667,33 @@ 

          return TestResultFailed(subject, self.test_case_name, self.scenario, result['id'])

  

  

+ class EmptyRule(Rule):

+     """

+     This rule is similar as PassingTestCaseRule, with the exception, that

+     no test results are being expected and evaluated. Result is passed,

+     every time decision_context and subject are matched.

+     """

+     yaml_tag = '!EmptyRule'

+ 

+     safe_yaml_attributes = {}

+ 

+     def check(

+             self,

+             policy,

+             product_version,

+             subject,

+             results_retriever):

+         return [NoTestsRequired(subject)]

+ 

+     def matches(self, policy, **attributes):

+         return True

+ 

+     def to_json(self):

+         return {

+             'rule': self.__class__.__name__

+         }

+ 

+ 

  class ObsoleteRule(Rule):

      """

      The base class for an obsolete rule.

@@ -20,7 +20,8 @@ 

      TestResultPassed,

      InvalidRemoteRuleYaml,

      MissingRemoteRuleYaml,

-     OnDemandPolicy

+     OnDemandPolicy,

+     NoTestsRequired

  )

  from greenwave.resources import ResultsRetriever

  from greenwave.safe_yaml import SafeYAMLError
@@ -778,6 +779,45 @@ 

              assert decision[0].is_satisfied is False

  

  

+ def test_remote_rule_policy_empty_rule(tmpdir):

+     subject = create_subject('koji_build', 'nethack-1.2.3-1.el9000')

+ 

+     serverside_fragment = dedent("""

+         --- !Policy

+         id: "taskotron_release_critical_tasks_with_remoterule"

+         product_versions:

+           - fedora-26

+         decision_context: bodhi_update_push_stable_with_remoterule

+         subject_type: koji_build

+         rules:

+           - !RemoteRule {}

+         """)

+ 

+     remote_fragment = dedent("""

+         --- !Policy

+         decision_context: bodhi_update_push_stable_with_remoterule

+         rules:

+           - !EmptyRule {}

Why not use rules: [] instead?

+         """)

+ 

+     p = tmpdir.join('gating.yaml')

+     p.write(serverside_fragment)

+     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 = remote_fragment

+             policies = load_policies(tmpdir.strpath)

+             policy = policies[0]

+ 

+             results = DummyResultsRetriever()

+             print(results)

+             decision = policy.check('fedora-26', subject, results)

+             print(decision)

+             assert len(decision) == 1

+             assert isinstance(decision[0], NoTestsRequired)

+             assert decision[0].is_satisfied is True

+ 

+ 

  def test_remote_rule_malformed_yaml(tmpdir):

      """ Testing the RemoteRule with a malformed gating.yaml file """

  

This fixes #603
JIRA: RHELWF-2189

@bgoncalv, @vcrhonek, @pingou I had discussion about this patch with @vmaljulin, but I need your opinion.

This patch would fix the issue #603 but packager would require to explicitly mention in their gating.yaml file that specific decision_context doesn't require any tests if they expect Greenwave to pass for it. Otherwise, if the gating.yaml file exists for given build but doesn't contain requested decision_context, Greenwave would still fail. This could help discover some typos in the file.

Does this sound OK? Or would it be better for Greenwave to assume that no tests are required (and pass) if it doesn't find given decision_context in gating.yaml?

@lholecek if I understood correct it seems fine to me :-)

It seems this won't affect existing gating.yaml format, like if decision context is missing it would fail gating, but just add a support to configure a decision context that wouldn't require any test.

This could affect the internal gating bot, as it just checks if gating.yaml exist to assume there are additional gating tests, but this would be an issue on the gating bot and not on greenwave.

@msrb @jkaluza what do you think?

As long as @bgoncalv agrees and there's no reaction from others, I think we can merge it. @lholecek @gnaponie what do you think?

@bgoncalv, @vcrhonek, @pingou I had discussion about this patch with @vmaljulin, but I need your opinion.

This patch would fix the issue #603 but packager would require to explicitly mention in their gating.yaml file that specific decision_context doesn't require any tests if they expect Greenwave to pass for it. Otherwise, if the gating.yaml file exists for given build but doesn't contain requested decision_context, Greenwave would still fail. This could help discover some typos in the file.

Does this sound OK? Or would it be better for Greenwave to assume that no tests are required (and pass) if it doesn't find given decision_context in gating.yaml?

IIUC: pass if no decision_context is found is the current behavior, right?
If so, if we want to change this, we will need to communicate this loud and
clear (likely communityblog + emails to the devel-announce list on the Fedora
side and other lists internally).

My wonder is: is the gain worth the pain?
If the idea is to find out gating.yaml are valid or not, could we provide a
CLI tool that packagers can run on their machine or that we can automatically
run on a regular basis (say daily) and report the output to the maintainers?

Why not use rules: [] instead?

I've checked if this would work and found that it works as expected i.e. if there is a remote rule with the matching decision_context and rules: [] it returns no tests are required. I've created tests for such configuration in PR#621.

Pull-Request has been closed by vmaljulin

3 years ago