-
Notifications
You must be signed in to change notification settings - Fork 30
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
Post comment with summary and details to GitHub issue #5
Comments
@whilefoo rfc on how we can deal with comment outputs. Perhaps we can have a standard recognized property on the output interface? Then the kernel can decide whether to pass it around or something. interface PluginOutput {
comment: string; // html comment
rewards: Rewards; // { "whilefoo": "500", "token": "0x0" } etc
} |
/start |
Tips:
|
This needs #7 to be merged first. Also probably needs ubiquity-os/permit-generation#2 to be able to generate the permits properly. |
I think you should fork from and overtake that second pull due to us being behind schedule |
there are a couple of options:
In theory 2. option sounds good to separate concerns but it's another plugin which means another call to github actions thus more latency, so for the sake of speed it'd go with option 1 or 3, but going with these 2 options would mean there will 1 comment for rewards summary and 1 comment for permits. |
To me 1 is the most straightforward to do for few reasons:
3 might make more sense in terms of architecture however. In such case the kernel should pass down results. It is more of an architecture question. Although, if we ever have other plugins in the flow that have influence on the total incentives, it would make sense to go through the kernel to aggregate the total result. |
I think the most pure architecture would be that plugins can NOT inherit (write) authentication (only read if possible) of the kernel. As a consequence, no plugin should be able to post directly any issue. Ideally it should only be the kernel with a direct interface to issues. Plugins should just output comment HTML and the kernel can post it all in a single comment at the end of the webhook event invocation chain.
Could be interesting to have a dedicated plugin to handle commenting, but because this seems like such an essential capability, I would more carefully consider the pros/cons of including this within the kernel.
I'm not concerned about latency now. Besides we can port any plugin to Cloudflare Workers (or combine several inside a single Worker) when we are ready to fix latency issues. To be honest though, except for setting labels, I have no problem with latency (like with permit generation) so there's no need to overengineer yet. I also love the GitHub Actions logs being available for easy debugging, and the fact that they are super generous with the compute. Given the entire conversation and all the considerations, I definitely think we should do option 3. This separates concerns the best, which will allow our plugin ecosystem to grow the fastest. |
I think each plugin should output JSON not html as it is not reliable to parse nor manipulate and requires Having a plugin handling commenting seems quite weird as commenting is done by calling Octokit and the REST api which is already a library by itself, so no need to encapsulate it within another one to do the same thing. My view on this, is to finalize ubiquity-os/permit-generation#5 to import it withing the |
I know JSON makes things more complicated than it needs to be with serialization/transport. Then we have problems like this #18. If the comment property is ONLY meant to be used for GitHub comments, then transporting HTML only is the most sensible. Besides, there's no other metadata thats necessary within the As a somewhat side note: there should also not be any variables inside of the HTML. We could look it as like server-side-rendering (or in this case, plugin-side-rendering) handle producing the final HTML output and then output it as a single string of html entities.
We are currently using
If we agree that this is considered as technical debt, and that we are accruing this so that we can get back on schedule, ok. |
If you want to manipulate and convey data, HTML really is not made for this. If you want something formatted similarly but made for data we can use XML format. Biggest advantage from this is to have the comment reward fully standalone, while easy to integrate with the kernel. If we do something that handles the comment it means each and every module has to send it there and that module should understand every different content / format we send which would be way easier if the module itself handled its own comments, formatting wise. |
Going back to my "plugin-side-rendering" mention, the data manipulation happens inside of the logic of the plugin. Then when the plugin is finally done with all of its compute, it emits a single string in the The type HtmlString = string;
type RequestedPermits = { username: string; amount: string; token: string; }[]
interface PluginOutput {
metadata: Record<string, any>;
comment: HtmlString;
rewards: RequestedPermits;
} |
But then how do we consider the formatting of that output? Practical case: we want to post a comment when a user queries a wallet. That comment is 'user name': 'wallet 0x0' Then, comment-reward wants to post the results. Should ask the Kernel to call the comment plugin, but then formatting is different. Should the Kernel notify the comment plugin that it wants a different output formatting? Should the Kernel compute beforehand the HTML and send it to the comment plugin? Might be something I don't grasp there. Because I do understand your use case but it seems to be very deterministic on what is the purpose of the plugins which kinda defeats the purpose of having plugins, looks more like a monolithic approach to me |
The proposed However, as we know, there will be situations where a subsequent plugin will consider the results of a previous plugin, which means it would need to change the comment that is rendered. In this situation the subsequent plugin should clobber the output of the previous plugin. It is now clear to be that this will be a new challenge to express to the kernel that it should ignore the comment output of a previous plugin. I suppose it would be straightforward in the metadata using the plugin ID i.e.
I think that comments should be handled from within the kernel. There should not be a separate comment plugin. Read my explanation here. InputsFor completeness of my previous comment: type PluginId = string; // i.e. `ubiquibot/conversation-rewards` `ubiquibot/assistive-pricing`
interface PluginInput {
metadata: Record<PluginId, any>;
context: GitHubEventContext;
} Notice that we should not pass in the HTML of other plugins. Instead, just the metadata ("computed") values from the previous plugins. |
This can work, but we skyrocket coupling and to me defeat purpose of plugins that should be unaware of each other. If any plugin has to understand the result of a previous plugin, it means these plugins have necessarily to co-exist so basically they become a single plugin with no purpose to split them. |
I understand your concern and I would need to put more thought into composability. Maybe the I need help to think through how any partner in the future can create new plugins that modify
I don't like the idea of having a single monolithic rewards generation plugin that wraps all the ways possible to compute rewards (i.e. time estimation, comment rewards, review rewards etc) |
@pavlovcik To mitigate that that's why inside the comment reward itself I also integrated that |
! action has an uncaught error |
I realized that to carry this task properly we need to handle flags for comment more delicately as they only indicate if the comment is |
I see, so you're suggesting that we must annotate each comment as well in order to properly handle the scoring at the end? Off hand I think there's:
I suppose we need a better naming convention for the pull related ones. They are considered as separate entities according to the GitHub api. They require different methods to obtain both types. |
Agreed, I think currently there are 3 possible things to annotate on the comments:
I think this shall cover all cases. |
Consider calling it "contributor" and "collaborator" as that is how it is presented on the GitHub APIs as I recall. Also I think you forgot about the "review comments" |
/stop |
+ You have been unassigned from the task |
/start |
Tips:
|
+ Evaluating results. Please wait... |
In the v1 of the Ubiquibot, when a result gets evaluated, a recap is posted to the related issue with a summary and details of the rewards as well as the link to the rewards themselves Example: ubiquity/cloudflare-deploy-action#9 (comment)
The same logic should be applied in the v2 version by creating a new Module responsible to post that comment. This module will receive a similar input than the one mentioned here
The module should be:
The text was updated successfully, but these errors were encountered: