Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RemoteRule more consistent #184

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

hluk
Copy link
Member

@hluk hluk commented Aug 29, 2023

Previously, a Greenwave policy with just a RemoteRule would cause "Cannot find any applicable policies" 404 error if the remote policy file does not contain a policy with the requested decision context. But that was not the case if the Greenwave policy would contain at least one PassingTestCaseRule.

New behavior is to pass the decision with "no tests are required" instead of the 404 error.

Decision responses will always include policies (in applicable_policies) that contain RemoteRule without checking if the remote gating file contains any matching policy.

Decision change messages will still only be published if there are any required tests.

Fixes https://pagure.io/greenwave/issue/668
JIRA: RHELWF-9620

@hluk hluk requested a review from mvalik August 29, 2023 13:26
@hluk
Copy link
Member Author

hluk commented Aug 29, 2023

I'm not entirely sure if this does not break anything.

@AdamWill
Copy link
Contributor

So...looking at the history here, I think an effect of this will be that we check for decision changes much more often. It seems like the original intent of this matches() concept was for the decision change messaging logic (man I still hate that stuff) - we use the concept of whether a policy "matches" the context/version/subject to decide whether we need to check if the decision has changed when we get a new waiver or test result.

I guess for updates that are gated on anything else - i.e. critpath updates - we will have to check the decision for any new waiver or result anyhow. But I'm wondering if perhaps for updates which aren't gated on anything else and whose package(s) don't have any gating.yaml set up, we will now wind up checking for a decision change every time a new result appears for a build in that update, whereas previously we wouldn't?

@AdamWill
Copy link
Contributor

To be clear I'm talking about an update like https://bodhi.fedoraproject.org/updates/FEDORA-2023-10d819be90 , for instance. Nothing gates that update, but we do run a CI test on it (for some updates I think we run multiple CI tests but none are gating). Right now I don't think we'd ever actually check for a decision change for that update, but with this change, I think we'd check for a decision change when we got the CI test result...

Previously, a Greenwave policy with just a RemoteRule would cause
"Cannot find any applicable policies" 404 error if the remote policy
file does not contain a policy with the requested decision context. But
that was not the case if the Greenwave policy would contain at least one
PassingTestCaseRule.

New behavior is to pass the decision with "no tests are required"
instead of the 404 error.

**Decision responses** will always include policies (in
`applicable_policies`) that contain RemoteRule without checking if the
remote gating file contains any matching policy.

**Decision change messages** will still only be published if there are
any required tests.

Fixes https://pagure.io/greenwave/issue/668
JIRA: RHELWF-9620
@hluk
Copy link
Member Author

hluk commented Sep 5, 2023

I've changed the behavior so that decision change message are published as before. Only the decision API response changes (always includes the matching policies containing RemoteRule).

@hluk
Copy link
Member Author

hluk commented Sep 21, 2023

@mvalik Please review.

@mvalik
Copy link
Contributor

mvalik commented Sep 25, 2023

+1

@hluk hluk merged commit cf9a2e0 into release-engineering:master Sep 26, 2023
11 checks passed
@hluk hluk deleted the remoterule-consistency branch October 20, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants