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

Automatic Merge In Timeout #20

Closed
0x4007 opened this issue Jun 5, 2024 · 17 comments · Fixed by ubiquity-os-marketplace/daemon-merging#1
Closed

Automatic Merge In Timeout #20

0x4007 opened this issue Jun 5, 2024 · 17 comments · Fixed by ubiquity-os-marketplace/daemon-merging#1

Comments

@0x4007
Copy link
Member

0x4007 commented Jun 5, 2024

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:

  1. Collaborator Minimum Approvals Required: 0
  2. Contributor Minimum Approvals Required: 1
  3. Collaborator Merge Timeout (Days): "3.5 days"
  4. 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.

Footnotes

  1. Omit or comment this out to disable this behavior

@0x4007
Copy link
Member Author

0x4007 commented Jun 17, 2024

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

@gentlementlegen
Copy link
Member

gentlementlegen commented Jul 2, 2024

I would suggest a few changes:

The configuration can be made like

  1. Collaborator Minimum Approvals Required: 0
  2. Contributor Minimum Approvals Required: 1
  3. Collaborator Merge Timeout: "3.5 days"
  4. 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.


Related repo: https://github.com/ubiquibot/automated-merging

@gentlementlegen
Copy link
Member

/start

Copy link

ubiquibot bot commented Jul 2, 2024

DeadlineTue, 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
Copy link
Member

rndquu commented Jul 3, 2024

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
Copy link
Member

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
Copy link
Member

rndquu commented Jul 3, 2024

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
Copy link
Member

gentlementlegen commented Jul 3, 2024

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
Copy link
Member

rndquu commented Jul 3, 2024

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

Sounds good

Copy link

ubiquibot bot commented Jul 29, 2024

+ Evaluating results. Please wait...

Copy link

ubiquibot-dev bot commented Jul 29, 2024

[ 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

Copy link

ubiquibot bot commented Jul 29, 2024

[ 51.4 WXDAI ]

@0x4007
Contributions Overview
ViewContributionCountReward
IssueSpecification148.2
IssueComment13.2
Conversation Incentives
CommentFormattingRelevanceReward
### 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
148.2
I assume it'll take a week in total including to also write unit...
3.20.63.2

[ 1228.8 WXDAI ]

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueTask11200
IssueComment30
IssueComment328.8
Conversation Incentives
CommentFormattingRelevanceReward
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.7914.6
I agree with your points. Albeit I think that's already how we k...
70.727
So wouldn't you consider it to be safe enough to if we: - enfor...
7.2
li:
  count: 3
  score: "3"
  words: 19
0.517.2

[ 12.6 WXDAI ]

@rndquu
Contributions Overview
ViewContributionCountReward
IssueComment312.6
Conversation Incentives
CommentFormattingRelevanceReward
> This plugin can be considered dangerous if not configured p...
2.70.452.7
> I agree with your points. Albeit I think that's already how...
9.7
code:
  count: 3
  score: "3"
  words: 3
0.779.7
> enforce at least one reviewer either case (collaborator, co...
0.20.740.2

@0x4007
Copy link
Member Author

0x4007 commented Jul 29, 2024

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

@gentlementlegen
Copy link
Member

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

@0x4007
Copy link
Member Author

0x4007 commented Jul 29, 2024

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
Copy link
Member

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
Copy link
Member Author

0x4007 commented Jul 29, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants