ubiquity-os-marketplace / text-conversation-rewards

1 stars 27 forks source link

Ensure Assignee on Linked Pull Merge #149

Closed 0x4007 closed 3 weeks ago

0x4007 commented 1 month ago

Maybe we could a feature if the issue has a linked PR that has been merged, it should assign the user to the issue and close it?

Originally posted by @0x4007 in https://github.com/ubiquity/pay.ubq.fi/issues/267#issuecomment-2399122707

zugdev commented 1 month ago

I would love to contribute to this repo and issue but I am very unsure how to run this thing. Therefore, I also have no idea on how to QA it. Is this an extension of ubiquity-os, should I run ubiquity-os-kernel?

0x4007 commented 1 month ago

As I understand you can add ubiquity-os to your own org and then configure the plugins to use your own plugin repo. This is not generally recommended because it uses our already very low daily KV limits but it could be useful in a pinch to get started until you figure out how to run everything locally.

@gentlementlegen rfc; also this makes sense why we kept running so close to finish our limits. Everybody is probably developing using our production bot. Perhaps we should whitelist plugin execution only from plugins within our orgs, if rate limits become a problem in the near future?

@sshivaditya2019 it would be really impressive if your bot could answer this

zugdev commented 1 month ago

Docs are kinda outdated, there are new environment variables, the config yml structure has changed a lot. Although the videos in wiki do help a lot, to run locally with real plugins is pretty hard. I think issues to document how to run locally should benefit the project a lot, not only because of DX but - as you said - not taking your limited KV run time.

The config.ts at ubiquity-os/ubiquity-os-kernel is specially confusing, and I don't know where current .ubiquibot-config.yml is at. I suppose it's at ubiquity-os/configuration-loader but that's 4 months old. Also config.ts mentions a repo that I can't view called ubiquibot-config.

Therefore I've hit a wall at:

image

I probably just need to bang my head more against the wall, but trying to improve this setup process for new contributors should help a lot :)

gentlementlegen commented 1 month ago

A valid configuration can be found within the quick start of the kernel: https://github.com/ubiquity-os/ubiquity-os-kernel?tab=readme-ov-file#plugin-quick-start

Also the errors message tells you that /plugins expects an array. Something like

plugins:
  - uses: ubiquity-os/conversation-rewards
0x4007 commented 1 month ago

Perhaps it would be wise of us to make our config public for the org for reference.

We have org specific encryption now so there shouldn't be any risk for sensitive values.

https://github.com/ubiquity-os/ubiquibot-config/blob/main/.github/.ubiquibot-config.yml

gentlementlegen commented 1 month ago

@0x4007 Each plugin has an example on how to be run within its README, I think we should rather point that out.

zugdev commented 1 month ago

@0x4007 Each plugin has an example on how to be run within its README, I think we should rather point that out.

I see that. I still got a couple of "micro" issues in my attempts to run it though. I had:

  1. Weird config.ts paths which led to 404

  2. With plugin secrets

  3. And about manifest.json being missplaced

I think perhaps:

  1. Clarifying in kernel that plugins needs manifest.json and a plugin-config.yml which are both located in the plugin's repo.

  2. Having plugin-config-example.yml?

  3. The secrets part of it can probably just be ignored, I believe you can QA a big part of the plugin's flow without reaching the end of it.

Also some of that might be available in some repo, but there are a lot of repos and it got pretty hard going back and forth trying to understand it all. A centralized piece of knowledge, wiki or readme, involving all you need to know on kernel and plugins should help.

I had a lot in my mind today but even while trying this all day I wasn't really able to run. Could be skill issue, though this usually doesn't happen ^^ ...

If my experience is an isolated case feel free to ignore it, but I think this slight improvement could help a lot of devs on working and QAing Ubiquity plugins.

zugdev commented 1 month ago

This issue @gentlementlegen proposed should improve a lot DX in terms of what I've discussed above.

https://github.com/ubiquity-os-marketplace/text-conversation-rewards/issues/102#issuecomment-2310531373

0x4007 commented 1 month ago

I also had a bad time trying to work on my first plugin so @gentlementlegen it would be helpful if you can straighten this out so that we can contribute.

@whilefoo @Keyrxng might also be able to fix

Keyrxng commented 1 month ago

gentlementlegen it would be helpful if you can straighten this out so that we can contribute.

I've had a horrible experience running tests, live QA and overall usage of this plugin if I'm honest couldn't run it properly so could never QA it properly either. I got the config correct eventually but encountered more issues beyond that

I also had a bad time trying to work on my first plugin so whilefoo Keyrxng might also be able to fix

You mean creating a new fresh plugin from scratch or work on this plugin sorry?

I do remember suggesting this feature a while ago if you mean to fix this I can start work on it and try to fix things up.

gentlementlegen commented 3 weeks ago

This feature seems very dangerous to me, as I could use an already merged pull-request and start linking it against every issue and get assigned + get the reward without any validation by anyone isn't it?

Keyrxng commented 3 weeks ago

Maybe we could a feature if the issue has a linked PR that has been merged, it should assign the user to the issue and close it?

This feature seems very dangerous to me, as I could use an already merged pull-request and start linking it against every issue and get assigned + get the reward without any validation by anyone isn't it?

I thought of something along the lines of:

  1. Try to close issue as complete > check if assignee
  2. If not assignee, check PR author of PR being merged > check last timeline assignee for task
  3. If author !== last assignee, throw error comment. Manually assign author, close as complete. (maybe auto-assign here too actually)
  4. If author === last assignee, auto-assign assignee, close as complete.

The only automated merging is through collaborator approval and only admins/collaborators should be able to actually merge a PR. It's usually the last on top that should get re-assigned and reward generated because if I'm unassigned from a task and someone tries to take it I let them know, I think most would too.

zugdev commented 3 weeks ago

This feature seems very dangerous to me, as I could use an already merged pull-request and start linking it against every issue and get assigned + get the reward without any validation by anyone isn't it?

I believe that wouldn't be an issue if the plugin runs on pull merge event, which is triggered by a thrusted user - who won't merge if user is improperly referencing multiple issues. This would also prevent the use of already merged PRs, which, even if updated, would not trigger the plugin. Additionally, we could add a safeguard to check if the pull only has one "Resolves #" message.

0x4007 commented 3 weeks ago

Actually, I'm closing as unplanned:

  1. Assignment should occur when a pull is linked.
  2. Daemon-disqualify must monitor activity on pull. If there are commits or comments, it should renew its timer.

When these are fixed, there is no reason for this scenario (no assignee on pull merge) should occur.