#465 Simplified policies syntax for remote policies
Opened 4 years ago by pingou. Modified 2 years ago

In the discussion about rawhide package gating on the fedora-devel list there was some feedback about the complexity of the gating.yaml file.

Would there be a way to simplify that file when loading remote policies? Maybe not all rules should/are supported there and thus not all fields apply, allowing us to simplify its structure?

Source: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/XMIBCNK4WT72XQGZRHOPD45YS4YYLTE6/


I highly disagree that TOML format is an improvement to YAML.

The very.long.reverse.domain.test.names are not ideal.

Yeah... Unfortunately, these names need to be unique which restricts how much we can shorten them.

Then there's decision_context which apparently does nothing
but has to be there.

This is critical for ensuring the correct rules are being evaluated for a given context, e.g. can this pkg go into staging? can this pkg be released?

Is there any rule type other than PassingTestCaseRule?

There are, but you're likely never to use them. We could make an improvement to default to this rule if no other one is provided.

Changing the format would just be a suicide. Everyone would need to change the name of the file from "gating.yaml" to "gating.toml" and it wouldn't be backward compatible. We have too many of them, also internally, to even consider this change, in my opinion.

Greenwave is unaware of the test case name. It can also be "test". So this is not really the place to discuss that (I'm not even sure what this place is).

decision_context does nothing from the final user (the one that writes the gating.yaml) point of view, but it does something from the Greenwave point of view and also the user that configures the policies in Greenwave conf. Lui well explained why this is needed. I don't see any better way to have this information and I don't think it's a big effort from the user point of view to keep that.

+1 to Lui's idea to have PassingTestCaseRule as default if not specified.

We might consider changing a bit the organization of the fields, I guess. Like remove "--- !Policy" and also work on that https://pagure.io/greenwave/issue/467 and that https://pagure.io/greenwave/issue/468

Greenwave would need to make some adjust a bit the content, but that's ok if it helps the user.

Changing the format would just be a suicide. Everyone would need to change the name of the file from "gating.yaml" to "gating.toml" and it wouldn't be backward compatible. We have too many of them, also internally, to even consider this change, in my opinion.

This was juts introduced in Fedora, so this is just an excuse. You can keep using whatever you need internally. It can also be done in a way that if toml is not found, it can fallback to yaml. (I'm not in favor of toml, I'm just pointing out that this argument is toothless.)

I don't think it's a big effort from the user point of view to keep that.

Every copy-pasta boilerplate is going to eventually be a big effort from the user point of view.

Obviously, it doesn't matter if I copy paste 5 or 20 lines of gibberish now, but if we want wide adaption of Gating in Fedora, the file should be:

  • as easy to read as possible
  • as easy to hack on as possible

Any "copy this line and ask no questions" kind of thing hurts both.

This was juts introduced in Fedora, so this is just an excuse. You can keep using whatever you need internally. It can also be done in a way that if toml is not found, it can fallback to yaml. (I'm not in favor of toml, I'm just pointing out that this argument is toothless.)

We would like to keep Greenwave instances as similar as possible.
We could check both files, but I don't see much difference between yaml and toml, they are both standard formats, yaml is pretty common and used. I'm ok with making the current format of the gating.yaml simpler, but I don't see a real advantage in choosing toml.
It's just a personal preference, if we change it, someone else will hate toml.

Every copy-pasta boilerplate is going to eventually be a big effort from the user point of view.
Obviously, it doesn't matter if I copy paste 5 or 20 lines of gibberish now, but if we want wide adaption of Gating in Fedora, the file should be:

as easy to read as possible
as easy to hack on as possible

Any "copy this line and ask no questions" kind of thing hurts both.

I would be happy to remove it if I would have another way to get this information. But I'm not sure there is one.
Please see @lucarval's comment for more context.

but I don't see a real advantage in choosing toml.

I agree. Simple yaml is fine. Not this one with class names or whatever that even is. Yaml can be both simple and overengineered. This one is overengineered.

It's just a personal preference, if we change it, someone else will hate toml.

Totally agree here.


The design fail here is the way you are thinking about this file:

  1. There's greenwave
  2. ... ?
  3. Config file in package scm reposiotry.

While here is how packagers (users of this) will think about it:

  1. I hava a package with a scm repository.
  2. I have tests to gate on.
  3. Let's put a config file with 2. into 1.

The machinery about this should make greenwave an implementation detail.

From the packager perspective, this needs to be configured:

  • To push to testing, gate on those tests: list of names
  • To push to stable, gate on those tests: list of names
  • (Possibly make it easy to not duplicate this info)

The example provided by Tom Hughes is almost perfect:

rules:
fedora-*:
- dist.abicheck
- dist.rpmlint
fedora-30:
- my.special.text

Except that the file is in scm, so the Fedora version is determine by branch if needed:

stable:
- dist.abicheck
- dist.rpmlint
- centos.jenkins.ci
testing:
- dist.abicheck

Everything else is a boilerplate.

We cannot have "stable" and "testing". We need to have the "decision_context". And since it's not Greenwave that decides that, but the user (who wrote the policies in the first place), you can put whatever you want, also "stable". Greenwave cannot possibly know that you wrote "stable" but you meant "bodhi_update_push_stable" or whatever it is.
It's a string completely decided by the user, so we cannot put some kind of "intelligence" on Greenwave's side, people can put really everything there.

fedora-*:
- bodhi_update_push_*: [dist.abicheck]
- bodhi_update_push_stable: [dist.rpmlint, centos.jenkins.ci]
- bodhi_update_push_testing: [something.else]
fedora-30:
- bodhi_update_push_*: [additional.test]

Your perspective is shaped by "we are greenwave". My perspective is shaped by "this is a Fedora package". If greenwawe doesn't know the latter, can that be fixed somehow?

My perspective is "there's no possible way I can implement that".
It's not Greenwave that decides what to put there, whoever is responsible of those policies can change the name from "bodhi_update_push_stable" to "stable". It is just a configuration, if you don't like it, you can change it.
Why can't that be changed?

if you don't like it, you can change it.

How?

My point is: how can I know that when you write "bodhi_update_push_stable" you actually mean "stable"?
There might be some decision_context that is "something_else_stable". And what's the mapping in this case? Is that the same of "bodhi_update_push_stable"? Is it another "stable"?
Try walking in my shoes... I cannot go inside the head of the packager and find out what she/he wanted to mean by writing "stable" with no context at all...
Greenwave didn't decide "bodhi_update_push_stable" in the first place, it's not hard-coded. So why can't you change it?

Greenwave didn't decide "bodhi_update_push_stable" in the first place, it's not hard-coded. So why can't you change it?

I bet somebody can change it. Not me. I have no idea where is this even defined. @pingou maybe?

Thanks. I cannot change that file, but others possibly can.

@churchyard, so we can infer the product version, for sure. We get to the gating.yaml from an NVR after all. (product_version is weird and it doesn't always map to something like fedora-30, but I think for this case, it might be ok).

I'd advise against not having a top level key in gating.yaml because this creates a format that makes it challenging to be extended. The top level rules key seems reasonable to me.

The decision_context is what allows maintainers to specify which tests are required for pushing to testing vs stable. I don't see a reliable way to infer that information. IMO it must be explicitly set by the maintainer.

If we were to design this file today from scratch, and based on the examples you provided, we could do something like this:

rules:
  {decision_context}:
  - {test1}
  - {test2}

NOTE that the decision_context and test case names in reality won't look as simple as in the example in previous comments. Here's a more realistic example:

rules:
  bodhi_update_push_stable:
  - some.not_so_short.test_name

There's also current support for specifying test_case_name and scenario. We could allow each item to be either a string or an object to account for this.

Is this more inline with what the community would like to see?

Is this more inline with what the community would like to see?

Definitively for me.

We get to the gating.yaml from an NVR after all.

That doesn't always work, take for example fedora-release or fedora-obsolete-packages.

But where did you get the gating.yaml exactly? Because it lives in a Fedora dist-git branch.

Getting back to this (after 3 years!). Looking at glibc gating.yaml file (https://src.fedoraproject.org/rpms/glibc/blob/f36/f/gating.yaml):

--- !Policy
product_versions:
  - fedora-*
decision_contexts:
  - bodhi_update_push_stable
  - bodhi_update_push_stable_critpath
subject_type: koji_build
rules:
  - !PassingTestCaseRule {test_case_name: fedora-ci.koji-build.tier0.functional}
  - !PassingTestCaseRule {test_case_name: baseos-qe.koji-build.scratch-build.validation}
--- !Policy
product_versions:
  - rhel-9
decision_context: osci_compose_gate
rules:
  - !PassingTestCaseRule {test_case_name: baseos-ci.brew-build.tier1.functional}
  - !PassingTestCaseRule {test_case_name: osci.brew-build.rebuild.validation}

So I assume that with the requested changes it would be written as:

bodhi_update_push_stable:
- fedora-ci.koji-build.tier0.functional
- baseos-qe.koji-build.scratch-build.validation
bodhi_update_push_stable_critpath:
- fedora-ci.koji-build.tier0.functional
- baseos-qe.koji-build.scratch-build.validation
osci_compose_gate:
- baseos-ci.brew-build.tier1.functional
- osci.brew-build.rebuild.validation

This would need to make sure the decision_context names do not conflict - "osci_compose_gate" is used in Red Hat instance of Greenwave.

Also, if you need to use "stable" instead of "bodhi_update_push_stable", either change the name in the policy file (https://pagure.io/fedora-infra/ansible/blob/main/f/roles/openshift-apps/greenwave/templates/fedora.yaml) or we could add a support for an additional parameter to RemoteRule to support a different name.

Does that sound good?

Definitely an improvement.

Login to comment on this ticket.

Metadata