Fixing the issue when status check per rule matches multiple actions

[charles7668]

Close #29628
rule

Test / Build*
Test / Build *
Test / Build 2*
Test / Build 1*

rule2

Test / Build*
Test / Build 1*

rule3

Test / Build*
Test / Build 1*
NotExist*

[wxiaoguang]

Related to:

• Disallow merge when required checked are missing #29143

I guess the if matchedCount != len(requiredContexts) was written intentionally? @MarkusAmshove would you like to take a look? 🙏

[charles7668]

Related to:

* [Disallow merge when required checked are missing #29143](https://github.com/go-gitea/gitea/pull/29143)

I guess the if matchedCount != len(requiredContexts) was written intentionally? @MarkusAmshove would you like to take a look? 🙏

not sure , but I think this fix hasn't broken the expected behavior of #29143

Based on intuition, a rule can match many actions, unlike writing one rule to match one action (in which case, we only need to write the full action name).

Perhaps this checking behavior can be refactored.

[wxiaoguang]

I proposed some examples in #29631 (comment).

I think the correct logic should be:

1. All "required" are satisfied
2. All "rules" are used

Sorry I think I misunderstood something. Ignore my comments ....

ps: It would be helpful to add some tests to clarify the behavior.

[MarkusAmshove]

Related to:

• Disallow merge when required checked are missing #29143

I guess the if matchedCount != len(requiredContexts) was written intentionally? @MarkusAmshove would you like to take a look? 🙏

The problem I fixed with that condition was that the function was returning success even if a required check hasn't run at all (e.g. no status check was submitted that matches a required pattern).
I guess I didn't think of a * pattern that catches multiple checks (so required 1, but matched 5).
The condition should still be correct, but it should test if matched is less than required, not unequal.
That should catch both cases

[charles7668]

Related to:

• Disallow merge when required checked are missing #29143

I guess the if matchedCount != len(requiredContexts) was written intentionally? @MarkusAmshove would you like to take a look? 🙏

The problem I fixed with that condition was that the function was returning success even if a required check hasn't run at all (e.g. no status check was submitted that matches a required pattern). I guess I didn't think of a * pattern that catches multiple checks (so required 1, but matched 5). The condition should still be correct, but it should test if matched is less than required, not unequal. That should catch both cases

I'm not sure, but the expected behavior is that every rule needs to match at least one action?

If using <, then the following case may be incorrect.

actions

Build 1
Build 2
Build 2t

rules

*Build*
*Build *
*Build 1*
*Build 2*

[MarkusAmshove]

Yeah I think I didn't communicate that well.

The deleted if in this PR is an early exit, so I was talking about the pending condition.

To put it the other way around:
If there is a required context that matches no available status that is successful, then the function should return pending.

Currently it is also returning pending if a context matches multiple checks and that is obviously a bug.

The test case for the bug I fixed would look like this:

actions (all successful)

Build 1
Build 2

rules

Build 1
Build 2
Build 3

For that the function returned overall successful without my change

[charles7668]

Hmm... I think perhaps one rule matching at least one action is correct.

In GitHub, it seems like one rule matches just one action.

But if Gitea allows wildcard characters, then it seems to allow one rule to match multiple actions.

In this case, we can't simply compare the rule count and the matched action count.

case

this is result for this case when using <

[MarkusAmshove]

I've taken the time to write some tests, which I should have done beforehand, and also changed the != to < to see if my assumption was correct.

Have a look at this commit (feel free to grab the tests into this PR): MarkusAmshove@adc885b
the only test failing is TestMergeRequiredContextsCommitStatus_MissingContextWithPattern
But I also can't guarantee that I'm not missing some cases :)

[charles7668]

I've taken the time to write some tests, which I should have done beforehand, and also changed the != to < to see if my assumption was correct.

Have a look at this commit (feel free to grab the tests into this PR): MarkusAmshove@adc885b the only test failing is TestMergeRequiredContextsCommitStatus_MissingContextWithPattern But I also can't guarantee that I'm not missing some cases :)

I'm just curious about this case. I think it should pass because when allowing wildcard characters, sometimes intersections are possible.

[MarkusAmshove]

None of the contexts have a wildcard character, did you mean to add a * somewhere?

[charles7668]

None of the contexts have a wildcard character, did you mean to add a * somewhere?

Like this, just simulating when multiple rules match the same action.

	givenStatuses := createStatuses(
		createSuccess("Build 1"),
		createSuccess("Build 2"),
		createSuccess("Build 2t"),
	)

	requiredContexts := createRequiredContexts(
		"Build 1*",
		"Build 2*",
		"Build*",
		"Build *",
	)

[MarkusAmshove]

Yes, that should result in success :-)

Maybe it's easier to change the logic to check each required context if it matches a check and if one doesn't, return pending. That way we don't have to count how many match