Automatic Merge In Timeout

0x4007 commented 5 months ago

Overview

We need to maintain a bias of always moving forward in Ubiquity.
This plugin can be considered dangerous if not configured properly, but the general idea is that if reviewers forget about a pull, it should automatically be merged in after a timeout period.
The clock is based on the last timeline activity event on that particular pull request.
This must be eligible to generate payment permits.

Default configs:

Collaborator Minimum Approvals Required: 0
Contributor Minimum Approvals Required: 1
Collaborator Merge Timeout (Days): "3.5 days"
Contributor Merge Timeout (Days): "7 days"[^1^]

Remarks

Worst case scenario, the pull can be reverted. If a pull is detected to be reverted, perhaps a stretch goal of this plugin would be to disable automatic merges again for pulls associated to the same task.
This would be a wildcard event plugin.
A condition that should be there before merge is that all Checks should be green as well, and of course no conflicts. [^1^]: Omit or comment this out to disable this behavior

0x4007 commented 4 months ago

I assume it'll take a week in total including to also write unit tests etc.

gentlementlegen commented 4 months ago

I would suggest a few changes:

The configuration can be made like

Collaborator Minimum Approvals Required: 0
Contributor Minimum Approvals Required: 1
Collaborator Merge Timeout: "3.5 days"
Contributor Merge Timeout: "7 days"

And this plugin should not need to be able to generate permits since that would be occurring within conversation-rewards on PR merging.

A condition that should be there before merge is that all Checks should be green as well, and of course no conflicts.

gentlementlegen commented 4 months ago

/start

ubiquibot[bot] commented 4 months ago




Deadline Tue, Jul 9, 11:06 AM UTC

Registered Wallet
0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED

Tips:

Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

rndquu commented 4 months ago

This plugin can be considered dangerous if not configured properly

This plugin is dangerous no matter how we configure it

if reviewers forget about a pull

We need a dedicated QA engineer

it should automatically be merged in after a timeout period

We mustn't allow auto merges of unreviewed and unQAed PRs

gentlementlegen commented 4 months ago

I agree with your points. Albeit I think that's already how we kinda manually do things, since we wait for one reviewer to go through at least before considering merging the pull request. For the QA part, this should be covered by the unit tests, for most cases.

I was thinking of eventually create a warning message before the automatic merge that tags the reviewers before the merge happens.

rndquu commented 4 months ago

I agree with your points. Albeit I think that's already how we kinda manually do things, since we wait for one reviewer to go through at least before considering merging the pull request. For the QA part, this should be covered by the unit tests, for most cases.

I was thinking of eventually create a warning message before the automatic merge that tags the reviewers before the merge happens.

This "automerge plugin" is implied to solve an issue of long reviews but it does so by removing reviews at all thus reducing security and code quality levels.

Long reviews should be solved either by a dedicated QA engineer either by PR comment incentives for outside collaborators.

Any unreviewed PR is basically a none 0 risk of exfiltrating any critical ubiquity organization secret (X25519_PRIVATE_KEY, UBIQUIBOT_APP_PRIVATE_KEY, NPM_TOKEN, etc...).

gentlementlegen commented 4 months ago

So wouldn't you consider it to be safe enough to if we:

enforce at least one reviewer either case (collaborator, contributor)
check for conflicts
check for full pass on Action checks

(I would love a QA team as well for sure)

rndquu commented 4 months ago

enforce at least one reviewer either case (collaborator, contributor)

Sounds good

ubiquibot[bot] commented 3 months ago

+ Evaluating results. Please wait...

ubiquibot-dev[bot] commented 3 months ago

[ 1372.6 WXDAI ]

@gentlementlegen

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1	1200
Issue	Comment	3	0
Review	Comment	26	172.6

Conversation Incentives

Comment	Formatting	Relevance	Reward
I would suggest a few changes: The configuration can be made lik…	0 content: p: count: 84 score: 1 code: count: 1 score: 1 wordValue: 0 formattingMultiplier: 0	0.75	-
I agree with your points. Albeit I think that's already how we k…	0 content: p: count: 69 score: 1 wordValue: 0 formattingMultiplier: 0	0.6	-
So wouldn't you consider it to be safe enough to if we: - enforc…	0 content: p: count: 44 score: 1 wordValue: 0 formattingMultiplier: 0	0.7	-
Resolves https://github.com/ubiquibot/plugins-wishlist/issues/20	0 content: p: count: 2 score: 1 wordValue: 0 formattingMultiplier: 0	0.3	-
So far it calls them in order but since the run order is not gua…	9.2 content: p: count: 23 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.2	1.84
I thought it would be better to have it here so it can be hosted…	14 content: p: count: 35 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.3	4.2
It was set for debugging purposes mostly, will remove it then.	4.4 content: p: count: 11 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.2	0.88
Removed within https://github.com/ubiquibot/automated-merging/pu…	1.2 content: p: count: 3 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.3	0.36
@0x4007 looks better imo, will update.	2.4 content: p: count: 6 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.3	0.72
Updated at https://github.com/ubiquibot/automated-merging/pull/1…	1.2 content: p: count: 3 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.2	0.24
Since they pull first then commit and push, I don't think there …	32 content: p: count: 80 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.5	16
Added comments within the readme and the code comments at https:…	5.6 content: p: count: 14 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.5	2.8
@0x4007 Knip is clean, it's just that the comment doesn't get de…	8.4 content: p: count: 21 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.3	2.52
This is the token that is provided by the Kernel to post on beha…	16.8 content: p: count: 42 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.4	6.72
@0x4007 Sorry I am debugging on that branch right now, will remo…	6.8 content: p: count: 17 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.2	1.36
It is used here https://github.com/ubiquity/ubiquibot-kernel/blo…	10.8 content: p: count: 27 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.5	5.4
It keeps track of the pull request to check updates for. Eventua…	14.4 content: p: count: 36 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.4	5.76
## QA run https://github.com/Meniole/automated-merging/pull/5 (a…	46 content: h2: count: 115 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.7	32.2
@whilefoo Addressed your comments just now, sorry about the dela…	4 content: p: count: 10 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.7	2.8
@rndquu I enforced one review for each following our [conversati…	31.2 content: p: count: 75 score: 1 a: count: 1 score: 1 code: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.5	15.6
@rndquu Agreed, will remove Worker related files.	2.8 content: p: count: 7 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.4	1.12
@rndquu I changed the token to be the one from the repo itself a…	14.4 content: p: count: 34 score: 1 a: count: 2 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.5	7.2
The comment means there will be no action taken because this eve…	32.4 content: p: count: 81 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.4	12.96
@whilefoo Correct, that is the intended behavior. I can try to m…	7.6 content: p: count: 19 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.7	5.32
@rndquu I think the problem is that it only runs on `comment…	16.8 content: p: count: 37 score: 1 code: count: 5 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.6	10.08
@rndquu My config is like this: ```yml - uses: …	19.6 content: p: count: 35 score: 1 code: count: 14 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.3	5.88
@rndquu It can just be like ```yml plugins: - uses…	31.2 content: p: count: 46 score: 1 code: count: 32 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.6	18.72
@rndquu my mistake, got confused while writing it, fixed my comm…	6.4 content: p: count: 15 score: 1 code: count: 1 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.7	4.48
@rndquu I'll merge this because it is needed for the v2, but fee…	12.4 content: p: count: 31 score: 1 wordValue: 0.2 formattingMultiplier: 2	0.6	7.44

[ 57.1 WXDAI ]

@0x4007

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	54.6
Issue	Comment	1	0.3
Review	Comment	7	2.2

Conversation Incentives

Comment	Formatting	Relevance	Reward
### Overview - We need to maintain a bias of always moving forwa…	54.60000000000000728 content: h3: count: 182 score: 1 wordValue: 0.1 formattingMultiplier: 3	1	54.6
I assume it'll take a week in total including to also write unit…	3 content: p: count: 15 score: 1 wordValue: 0.2 formattingMultiplier: 1	0.1	0.3
```suggestion with: approvalsRequired: …	3.5 content: p: count: 21 score: 1 code: count: 14 score: 1 wordValue: 0.1 formattingMultiplier: 1	-	-
Defaults should be clearly documented in the readme so that ther…	4.1 content: p: count: 39 score: 1 a: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.5	2.05
Watching labeling pulls seems unusual since we never do. We do h…	1.5 content: p: count: 15 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.1	0.15
How is this used in the kernel side? If it's looking for this st…	2.4 content: p: count: 24 score: 1 wordValue: 0.1 formattingMultiplier: 1	-	-
What's the database used for	0.5 content: p: count: 5 score: 1 wordValue: 0.1 formattingMultiplier: 1	-	-
```suggestion "description": "Conditionally merge …	1.8 content: p: count: 9 score: 1 code: count: 9 score: 1 wordValue: 0.1 formattingMultiplier: 1	-	-
As long as you tested it works I think just merge and we can tes…	1.9 content: p: count: 19 score: 1 wordValue: 0.1 formattingMultiplier: 1	-	-

[ 43.96 WXDAI ]

@rndquu

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	8.4
Review	Comment	12	35.56

Conversation Incentives

Comment	Formatting	Relevance	Reward
This plugin is dangerous no matter how we configure it We need a…	2.6 content: p: count: 26 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.8	2.08
This "automerge plugin" is implied to solve an issue of long rev…	7 content: p: count: 67 score: 1 code: count: 3 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.9	6.3
Sounds good	0.2 content: p: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.1	0.02
@gentlementlegen Does the kernel wait for all plugins to be exec…	2.9 content: p: count: 29 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.2	0.58
@gentlementlegen From https://michaelheap.com/github-actions-ch…	3.3 content: p: count: 30 score: 1 code: count: 3 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.7	2.31
Oh, my bad, the `MEMBER` check actually exists, so every…	1.4 content: p: count: 13 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.6	0.84
@gentlementlegen Check [this](https://github.com/rndquu-org/test…	20.3 content: p: count: 112 score: 1 a: count: 1 score: 1 code: count: 90 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.2	4.06
@gentlementlegen One more thing, check [this](https://github.com…	9.8 content: p: count: 59 score: 1 a: count: 1 score: 1 code: count: 38 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.9	8.82
I meant that we should not set 0 values for the ubiquity organiz…	6.3 content: p: count: 60 score: 1 code: count: 1 score: 1 a: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.9	5.67
@gentlementlegen Help Trying to run the plugin with [this](https…	5.4 content: p: count: 48 score: 1 a: count: 3 score: 1 code: count: 3 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.6	3.24
The error is gone but the PR is not merged. Check [this](https:/…	5.1 content: p: count: 41 score: 1 a: count: 4 score: 1 code: count: 6 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.9	4.59
@gentlementlegen Check [this](https://github.com/rndquu-org/test…	3.9 content: p: count: 32 score: 1 a: count: 7 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.8	3.12
Could you provide a valid config for QA?	0.8 content: p: count: 8 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.8	0.64
How should the whole config look like with `pull_request.ope…	1.7 content: p: count: 16 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.7	1.19
How `user-activity-watcher` is connected with the `a…	1 content: p: count: 8 score: 1 code: count: 2 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.5	0.5

[ 11.28 WXDAI ]

@whilefoo

Contributions Overview

View	Contribution	Count	Reward
Review	Comment	11	11.28

Conversation Incentives

Comment	Formatting	Relevance	Reward
shouldn't approval count be higher than the setting?	0.8 content: p: count: 8 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.5	0.4
shouldn't this be in the env?	0.6 content: p: count: 6 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.3	0.18
is `lastActivity` even used anywhere? I can only see it'…	1.3 content: p: count: 12 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.6	0.78
and then how does this test pass if `lastActivity` is no…	1.4 content: p: count: 13 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.6	0.84
actually I'm worried about concurrency, like when two events tri…	4.5 content: p: count: 45 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.8	3.6
```suggestion if ((await getApprovalCount(co…	1.6 content: p: count: 8 score: 1 code: count: 8 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.4	0.64
Here you are passing auth token for the repo that triggered this…	4.1 content: p: count: 40 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.4	1.64
yes it should be github token provided by the action because you…	4.1 content: p: count: 40 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.4	1.64
@gentlementlegen have you seen my comments?	0.6 content: p: count: 6 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.7	0.42
Even though that may seem like an error it isn't, the code shoul…	3.3 content: p: count: 33 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.2	0.66
It looks like the plugin didn't even run but in fact it did run …	2.4 content: p: count: 23 score: 1 code: count: 1 score: 1 wordValue: 0.1 formattingMultiplier: 1	0.2	0.48

ubiquibot[bot] commented 3 months ago

[ 51.4 WXDAI ]

@0x4007

Contributions Overview

View	Contribution	Count	Reward
Issue	Specification	1	48.2
Issue	Comment	1	3.2

Conversation Incentives

Comment	Formatting	Relevance	Reward
### Overview - We need to maintain a bias of always moving fo...	48.2 h3: count: 3 score: "3" words: 4 li: count: 11 score: "11" words: 139	1	48.2
I assume it'll take a week in total including to also write unit...	3.2	0.6	3.2

[ 1228.8 WXDAI ]

@gentlementlegen

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1	1200
Issue	Comment	3	0
Issue	Comment	3	28.8

Conversation Incentives

Comment	Formatting	Relevance	Reward
I would suggest a few changes: The configuration can be made ...	- li: count: 4 score: "0" words: 25 code: count: 1 score: "0" words: 2 hr: count: 1 score: "0" words: 0	0.79	-
I agree with your points. Albeit I think that's already how we k...	-	0.72	-
So wouldn't you consider it to be safe enough to if we: - enfor...	- li: count: 3 score: "0" words: 19	0.51	-
I would suggest a few changes: The configuration can be made ...	14.6 li: count: 4 score: "4" words: 25 code: count: 1 score: "1" words: 2 hr: count: 1 score: "1" words: 0	0.79	14.6
I agree with your points. Albeit I think that's already how we k...	7	0.72	7
So wouldn't you consider it to be safe enough to if we: - enfor...	7.2 li: count: 3 score: "3" words: 19	0.51	7.2

[ 12.6 WXDAI ]

@rndquu

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	12.6

Conversation Incentives

Comment	Formatting	Relevance	Reward
> This plugin can be considered dangerous if not configured p...	2.7	0.45	2.7
> I agree with your points. Albeit I think that's already how...	9.7 code: count: 3 score: "3" words: 3	0.77	9.7
> enforce at least one reviewer either case (collaborator, co...	0.2	0.74	0.2

0x4007 commented 3 months ago

@gentlementlegen my formatting score isn't fully using the decimal library. Look at the tiny fraction leftover.

gentlementlegen commented 3 months ago

@0x4007 It is using it but one value must be used after it was already computed as a basic number. Adding a ticket.

0x4007 commented 3 months ago

I think it's useful to roll up the problems into tasks roughly weekly or whenever somebody starts on them.

Important to update specification and time estimate in every batch.

gentlementlegen commented 3 months ago

Agreed, just sometimes it is hard to know if some issues are related or not. Problem is grouping them too much slows down their solving, splitting them too much might introduce redundant tasks. Nevertheless, created a ticket for that one.

0x4007 commented 3 months ago

The developer working on the sprint will be able to determine if they are related but I suppose we generally offer credit either way for multiple tasks (we assume/credit for they are always not redundant by default) if the specification author doesn't know from the start.

Deadline	Tue, Jul 9, 11:06 AM UTC
Registered Wallet	0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED

ubiquity-os / plugins-wishlist