prow/plugins: prioritise repo level triggers over org level ones

[MadhavJivrajani]

After #30048 was merged, I wasn't sure why /retest triggers were not working, I think this is why.

So far, TriggerFor prioristised returning org level trigger configs. There might be cases where in a trigger would need to be configured specifically for a repo (ex: allowing /retest to trigger GH workflows). In cases like this, we should return the trigger that is defined at the repo level, rather than its corresponding org level trigger.

We could "work around" the above constraint by simply defining the repo level trigger higher up in the config, but then this trigger config would shadow the org level trigger, which is not desirable.

This change returns a repo level trigger if it finds one, if not, it returns the first org level trigger it sees (keeping in sync with the previous behaviour).

Fwiw - I'm not sure why multiple org level or multiple repo level triggers for the same org or repo would be specified.

/sig contributor-experience
/sig testing
/assign @stevekuznetsov

[MadhavJivrajani]

/hold
I'd also like sign-off on this by the GH admins.

[Priyankasaggu11929]

Hey @MadhavJivrajani, thanks for the PR.

Just trying to understand this better:

1. Did the /retest command fail for add generic methods for integer package utils#280 (comment) because it went over the 30-day limit (of GH workflow retrigger)?
   Or because of the "org-level trigger" taking precedence over the "repo-level trigger"?
   Have we tested the later scenario?

2. If we prioritize the repo-level trigger (like /retest) in cases like add generic methods for integer package utils#280 (comment), will it solve the problem of retriggering GitHub workflows even if it has been more than 30 days since the initial trigger?

   (I think no, based on what you mentioned earlier about the GH actions log retention policy)

   Or does /retest create a new run instead of retriggering the old GH action?

[MadhavJivrajani]

@Priyankasaggu11929 excellent points!
I was thinking about this a little more.

Regarding your first point, I thought I had /retested on multiple PRs, but evidently not. So to reconfirm my hypothesis, here's a PR I created for testing purposes to show that /retest is not working: kubernetes/utils#288

Regarding your second point, I agree that this might not help if the time window is outside of 30 days. What I thought we could do is: check if we get an appropriate error from the API, if we do, use prow to comment on the PR saying workflow is older than 30 days and link to GH docs, with a suggestion of what could be done.
Fwiw -- I do see that according to the GH API docs, the 2 error codes possible are 201 and 403, so maybe 403?

[stevekuznetsov]

We have a number of other places where either org or repo-level config is allowed, and I think our convention is to make the repo-level config win out, so the intent here makes sense. This also seems to parallel what we do in ApproveFor and LgtmFor above. I can't say if this change to TriggerFor would be breaking in some way - @cjwagner can you think of anything?

@MadhavJivrajani can you make the implementation in TriggerFor match those in the other functions, for cohesiveness?

[MadhavJivrajani]

@cjwagner 👋🏼
In case you can take a look here!

[cjwagner]

I can't say if this change to TriggerFor would be breaking in some way - @cjwagner can you think of anything?

It could technically be breaking since it does change behavior, but I can't imagine this being surprising. The only configs that would be affected by this probably intended for this to be the behavior from the beginning so this seems more like a fix.

@cjwagner 👋🏼
In case you can take a look here!

Generally LGTM, but please address Steve's feedback, especially the bit about matching the implementation in the other functions.

[MadhavJivrajani]

Thanks @cjwagner!

@stevekuznetsov I've changed the implementation to how LgtmFor and ApproveFor implement it, could you PTAL? Thank you!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MadhavJivrajani, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• prow/plugins/OWNERS [stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[MadhavJivrajani]

/hold cancel