ubiquity / ubiquibot-kernel

0 stars 11 forks source link

Configuration validation and annotations on errors #80

Open gentlementlegen opened 1 month ago

gentlementlegen commented 1 month ago

In the bot v1, when the configuration is changed, annotations are added to the configuration file if any error is encountered within the configuration. It would be really nice to have for v2 as it is a common scenario to have an invalid configuration and have no feedback about it because the error would only be visible within the worker logs.

The main challenge is that only plugins are aware of their configuration shape, so probably the kernel should call every plugin to check their validity, which would require an endpoint or some access to the configuration validators. At the same time, the manifest.json validity could be checked, relating to https://github.com/ubiquity/ubiquibot-kernel/issues/78.

Since this might imply quite a few network requests, we can also consider having this functionality as a separate Worker using service binding.

0x4007 commented 1 month ago

Since this might imply quite a few network requests

I think its fine given that we can make 5000 requests per hour, per organization, before we get rate limited as an app. Lets keep it simple and not do the separate Worker. Config changes do not happen regularly.

gentlementlegen commented 1 month ago

@0x4007 What you say is valid for the GitHub API. But Workers are limited to 50 requests per run / instance.

0x4007 commented 1 month ago

We can do this in a plugin instead?

gentlementlegen commented 1 month ago

I thought about it, but it means that the plugin should have access to the private repo containing the configuration and should be able to read / write which seems dangerous. Or maybe we can run that plugin within the configuration repo itself. But if we do so we cannot handle per repo based configurations I think

0x4007 commented 1 month ago

Or maybe we can run that plugin within the configuration repo itself.

Cool idea

But if we do so we cannot handle per repo based configurations I think

Research will reveal the answer!

gentlementlegen commented 1 week ago

/start

ubiquity-os[bot] commented 1 week ago
Warning! This task was created over 33 days ago. Please confirm that this issue specification is accurate before starting.
DeadlineThu, Sep 5, 6:39 AM UTC
Registered Wallet 0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED
Tips:
<ul>
<li>Use <code>/wallet 0x0000...0000</code> if you want to update your registered payment wallet address.</li>
<li>Be sure to open a draft pull request as soon as possible to communicate updates on your progress.</li>
<li>Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.</li>
<ul>
gentlementlegen commented 1 week ago

What should be done beyond the kernel side changes: Each plugin should have an entry point for validation on Workers, and an Action script for Actions.

0x4007 commented 1 week ago

Each plugin should have an entry point for validation on Workers, and an Action script for Actions.

Can you elaborate this isn't clear to others

gentlementlegen commented 1 week ago

The kernel workflow should be the following:

  1. The kernel receives a push event
  2. The kernel figures out it is the configuration file
  3. The kernel will, for each plugin inside the configuration, poke and endpoint for Workers and an Action for Actions that will parse the configuration and return validation errors
  4. I think the kernel should also put a warning on endpoints it cannot reach and ignore the plugin without breaking the whole configuration
  5. The kernel will eventually post annotation if any issue is found, or a message saying the configuration is valid

The v1 had a similar workflow. I think this is important to have this functionality specially for newcomers that could be confused about the configuration, or avoid breaking it during development and updates.

gentlementlegen commented 1 day ago

@0x4007 I've seen you mentioning an issue about the base multiplier having changed for v2, and indeed it was reset to 1. Put it back to 3 as it was in v1.