ubiquity-os-marketplace / daemon-merging

Automatically merges pull-requests that do not have activity beyond a certain deadline.
0 stars 7 forks source link

Workflow proposal for automated runs #3

Closed gentlementlegen closed 2 months ago

gentlementlegen commented 2 months ago

Triggering a run on a organization event implies a few issues when running this plugin:

Proposal

We already had a long talk about not having cron tasks. However I think it is suitable for this plugin, because:

To achieve so, my proposal would be to have a separate workflow which only purpose is to trigger a run once a day. If no pull-request is found, the workflow would get disabled thus not run anymore. On the event pull_request.opened or pull_request.reopened, the workflow would be enabled again. One run every 24 hours seems reasonable and realistic for our needs.

0x4007 commented 2 months ago

No for the same reasons we discussed. To repeat my main concern, imagine a future where we have 1000 partners. maybe 5 will be power users, maybe 100 will be "average" users, and the other 895 will be dead orgs. cron is horrible for scaling for this reason. You're wasting a ton of resources on the dead orgs.

gentlementlegen commented 2 months ago

On the dead orgs, the CRON would never run, so I am not sure where resources are being wasted? Plus given the use case the company with power users will maybe trigger 5000 runs a day, which probably will hit GitHub API limits and saturate the Action runs. Plus if they are not running with open source, they use all their Action quota quickly.

0x4007 commented 2 months ago

On the dead orgs, the CRON would never run, so I am not sure where resources are being wasted? Plus given the use case the company with power users will maybe trigger 5000 runs a day, which probably will hit GitHub API limits and saturate the Action runs. Plus if they are not running with open source, they use all their Action quota quickly.

The limit is 5000 per hour. We can make optimizations if some day in the far future it becomes a tangible problem.

gentlementlegen commented 2 months ago

Sure. Also remember that each run is one KV use as well, which is 1000 a day if I recall.

0x4007 commented 2 months ago

On the dead orgs, the CRON would never run

This also does not make sense to me. The purpose of the CRON is to check if it should react, and then to react. Events based does not need to check, it just reacts.

With CRON you need to check every org for if the job needs to react.

gentlementlegen commented 2 months ago

The purpose of the CRON is to trigger a run, which if it realizes that there is nothing to do, will disable the CRON.

Every time the workflow is triggered all the orgs and repos and checked either way, not sure what you mean by this.

0x4007 commented 2 months ago

Every time the workflow is triggered all the orgs and repos and checked either way

This is wrong, it should only check the organization which invoked the event.

gentlementlegen commented 2 months ago

It reads the configuration where you can specify what organization / repos you want to be watched. Otherwise we cannot support cross organization or non-organization repositories.

0x4007 commented 2 months ago

Otherwise we cannot support cross organization or non-organization repositories.

I don't understand this but the only time/place that an update should be made by the bot is where activity is occurring from human contributors.

gentlementlegen commented 2 months ago

What I mention is in this pull request: https://github.com/ubiquibot/automated-merging/pull/4

0x4007 commented 2 months ago

Otherwise we cannot support cross organization or non-organization repositories.

Let's not do this. I think it simplifies implementation a lot if we only manage the active organization with the event source. I think its very unusual that a single company has multiple GitHub organizations. We do because we're developing a product intended to be used on GitHub and it was originally just for GitHub QA testing.

ubiquibot[bot] commented 2 months ago
# Issue was not closed as completed. Skipping.
ubiquibot-dev[bot] commented 2 months ago
# Issue was not closed as completed. Skipping.
0x4007 commented 2 months ago
+ Evaluating results. Please wait...

@gentlementlegen I noticed that it posted a message "evaluating results" but it shouldn't do that work at all when closed as not planned. We should also change the message to something like:

# No rewards because issue was closed as not planned.
gentlementlegen commented 2 months ago

Okay I can make a ticket on that regard, will take care of it.