Closed Keyrxng closed 1 month ago
I think there is a logic issue. Take the following scenario:
In the configuration, I set allowedReviewerRoles
to ["OWNER"]
. When the merge timeout will try to get the deadline requirements, a MEMBER
would be considered as a COLLABORATOR
instead of CONTRIBUTOR
because COLLABORATOR
is not in the allowedReviewerRoles
.
Example run: https://github.com/Meniole/automated-merging/actions/runs/10748866022/job/29813121322#step:6:68
I think there is a logic issue. Take the following scenario: In the configuration, I set
allowedReviewerRoles
to["OWNER"]
. When the merge timeout will try to get the deadline requirements, aMEMBER
would be considered as aCOLLABORATOR
instead ofCONTRIBUTOR
becauseCOLLABORATOR
is not in theallowedReviewerRoles
.Example run: https://github.com/Meniole/automated-merging/actions/runs/10748866022/job/29813121322#step:6:68
I'm a little confused by what you are implying. I think you are suggesting that the deadline requirement array should be the same as it was before that way they'd be assigned COLLABORATOR
and not the default CONTRIBUTOR
in the event that the allowedReviewerRoles
does not contain COLLABORATOR
, is that right @gentlementlegen?
With the following configuration
- uses:
- plugin: ubiquity/automated-merging@development
with:
approvalsRequired:
collaborator: 1
mergeTimeout:
collaborator: "2 minutes"
repos:
monitor: [ ]
ignore: ["conversation-rewards"]
allowedReviewerRoles: ["OWNER"]
Someone with the COLLABORATOR
role would not be able to be considered to be counted within the reviewers, which is ok and intended. However if the collaborator is the one opening the pull-request, its timing for closing would be the ones for contributor
instead of collaborator
because the array considered to check the merge timeout is only OWNER
. Hope it is clear enough.
What is an "owner" role? There should only be, basically: admin, assignee, collaborator, contributor.
Is owner the pull request creator?
OWNER
is the creator of the repository. I took this as an arbitrary example, same would apply if I used ADMIN
, that would be considered as a CONTRIBUTOR
for merging timeout in that example.
With the following configuration
- uses: - plugin: ubiquity/automated-merging@development with: approvalsRequired: collaborator: 1 mergeTimeout: collaborator: "2 minutes" repos: monitor: [ ] ignore: ["conversation-rewards"] allowedReviewerRoles: ["OWNER"]
Someone with the
COLLABORATOR
role would not be able to be considered to be counted within the reviewers, which is ok and intended. However if the collaborator is the one opening the pull-request, its timing for closing would be the ones forcontributor
instead ofcollaborator
because the array considered to check the merge timeout is onlyOWNER
. Hope it is clear enough.
Not sure how else to address the scenario that you raised apart from retaining your original implementation and hardcoding the timeout roles.
allowedReviewerRoles
item and additional unneeded complexity adding a new config item for this array separately.skipBotsEvents
should be as oppose to author defined.Can you fix your yarn I can't find your comment rn but didn't we agree that we'd standardize this? No harm done including it but potential room for error without it. I stated it as an addition in the spec body too.
Don't understand your message. What's the problem?
Seems that it works properly, my last QA: https://github.com/Meniole/automated-merging/actions/runs/10828231151/job/30043117944#step:6:68 Automated merge: https://github.com/Meniole/automated-merging/pull/19
Don't understand your message. What's the problem?
I don't understand your message now lmao.
allowedReviewerRoles
to always be the same for fetching timeouts and fetching authorized PR approvals, they appear like they'd always rely on the same roles. I scratched my head with a solve and revert to original was the only guaranteed way to avoid the scenario raised.packageManager
then the docs detail things better than I do but it was a prominent issue when we were using multiple yarn versions across plugins and packages etc. I'm sure we standardized to yarn@v1.22
org wide though so it makes sense to declare that I thoughtThis feature simplifies two core workflows:
It eases new contributor onboarding, since they won't have to follow system-specific installation processes anymore just to have the package manager you want them to.
It allows you to ensure that everyone in your team will use exactly the package manager version you intend them to, without them having to manually synchronize it each time you need to make an update.
@gentlementlegen why did UbiquityOS handle the merge?
Would be funny if it read the conversation and picked up on the vibes to merge.
@0x4007 I noticed that the configuration was set to 5 minutes for collaborators, probably for testing, I corrected that afterwards so it doesn't happen again. Maybe we could do sentiment analysis and eventually merge lol
+ ~ * checking vibes, please wait... * ~ +
Resolves #18
allowedRoles
previously used for assigning the timeout requirementsyarn format
made a few changespackage_manager
field topackage.json