ubiquity-os / ubiquity-os-kernel

1 stars 13 forks source link

feat: config event handler #18

Closed whilefoo closed 6 months ago

whilefoo commented 7 months ago

Previous version of workflow dispatch defined exact inputs, but I'm wondering is that really necessary. Can't we just send the whole webhook event along with settings from the config?

0x4007 commented 7 months ago

I know that there are limits around how many input parameters we can have. I assume that there are limits on character length per input parameter.

Let's try passing in everything in a single input parameter and if it hits a character limit then we can break it apart across several input parameters.

whilefoo commented 7 months ago

I know that there are limits around how many input parameters we can have. I assume that there are limits on character length per input parameter.

I've just tried invoking a workflow with event input which is stringified JSON webhook, and settings which is also stringified and it worked!

0x4007 commented 7 months ago

Is there an issue we can associate with this pull request? I'm a bit lost on context after reviewing tens of notifications.

Also looks like you can add some of those words to the cspell dictionary. There seems to be at least one genuine type "WATING"

whilefoo commented 7 months ago

I'm still not sure about the plugin input and output data.

Currently the first plugin receives this data:

{
    id: string; // reference to kernel KV
    eventName: string;
    event: string; // event payload
    settings: string;
    authToken: string;
    ref: string;
}

The plugin needs to return the following data:

{
    id: string; // reference to kernel KV
    owner: string; // owner of the repo that triggered the event
    repo: string;  // repo that triggered the event
    output: any; // output of the plugin
}

Should we let the first plugin dictate the input of the second plugin but some properties are essential for every plugin. For example the id, settings of the plugin, ref and maybe eventName and name too.

I'm thinking about a few different ways:

  1. We merge output data of the previous plugin with the essential data like this:
    {
    ...pluginOutput.output,
    id: pluginOutput.id,
    settings: JSON.stringify(nextPlugin.with),
    ref: nextPlugin.plugin.ref,
    }
  2. We have a separate field for that
    {
    id: pluginOutput.id,
    settings: JSON.stringify(nextPlugin.with),
    ref: nextPlugin.plugin.ref,
    previousPluginOutput: pluginOutput.output,
    }
  3. Same inputs for all plugins but additional data for output of the previous plugin:
    {
    id: string;
    eventName: string;
    event: string; // event payload
    settings: string;
    authToken: string;
    ref: string;
    previousPluginOutput: any, // this will be null for the first plugin
    }
0x4007 commented 7 months ago

I can't answer specifically because I don't feel that I have enough context (I didn't run the code)

Generally speaking here are some thoughts:

  1. I learned that public repo actions can have unlimited inputs, and private is 10 inputs (we don't need to worry about this for now)

To me this means that we do not have to be conservative, so feel free to pass all the data you might need (especially for this v1)

  1. Intuitively I feel that each plugin should respond to the kernel, and then the kernel should invoke the next plugin; instead of a plugin daisy chaining directly into another plugin. Because that means that the plugin developers would need to actually redundantly include the daisy chain logic (and some plugins may never be used in chains!)

  2. In order to track state (for when the bot receives a plugin response) I'm wondering if it's sufficient to utilize 1. Installation ID (represents which partner is using the bot) and 2. The repo ids that host each plugin

With those combined two values perhaps that's enough context for the bot to pick up where the plugins left off.

Otherwise if more information is needed (like repo, and who invoked the event) possibly just capturing and passing around the entire webhook event that invoked the whole chain of events. It's a large object but it has details around everything relevant to what invoked the event, like org, repo, comment url, user etc

0x4007 commented 7 months ago

Hey @whilefoo its 1 March. How's this looking for merging in?

whilefoo commented 7 months ago

I just need to add the code to get outputs from previous plugin to the input of next plugin and make sure everything is working and we can merge

0x4007 commented 7 months ago

When will you have that finished by?

github-actions[bot] commented 6 months ago
Coverage Report (0%) 
File% Stmts% Branch% Funcs% LinesUncovered Line #s
github-actions[bot] commented 6 months ago

Unused files (1)

src/github/types/webhook-events.ts

Unused dependencies (4)

Filename dependencies
package.json @octokit/webhooks-types
create-cloudflare
octokit
universal-github-app-jwt

Unused devDependencies (6)

Filename devDependencies
package.json @mswjs/databr/>`@types/jest`<br/esbuild
eslint-config-prettier
eslint-plugin-prettier
ts-node

Unlisted dependencies (8)

Filename unlisted
src/github/github-client.ts @octokit/corebr/>`@octokit/types`<br/@octokit/plugin-paginate-restbr/>`@octokit/plugin-rest-endpoint-methods`<br/@octokit/plugin-retrybr/>`@octokit/plugin-throttling`<br/@octokit/auth-app
tests/main.test.ts @jest/globals

Unlisted binaries (4)

Filename binaries
package.json lsof
awk
.github/workflows/build.yml build
.github/workflows/cspell.yml format:cspell
0x4007 commented 6 months ago

Previous version of workflow dispatch defined exact inputs, but I'm wondering is that really necessary. Can't we just send the whole webhook event along with settings from the config?

This was a naive implementation so you would know better.