Skip to content
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

Ensure Assignee on Linked Pull Merge #149

Closed
0x4007 opened this issue Oct 8, 2024 · 14 comments
Closed

Ensure Assignee on Linked Pull Merge #149

0x4007 opened this issue Oct 8, 2024 · 14 comments

Comments

@0x4007
Copy link
Member

0x4007 commented Oct 8, 2024

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 ubiquity/pay.ubq.fi#267 (comment)

@zugdev
Copy link

zugdev commented Oct 9, 2024

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
Copy link
Member Author

0x4007 commented Oct 9, 2024

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
Copy link

zugdev commented Oct 9, 2024

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
Copy link
Member

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
Copy link
Member Author

0x4007 commented Oct 10, 2024

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
Copy link
Member

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

@zugdev
Copy link

zugdev commented Oct 10, 2024

@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
Copy link

zugdev commented Oct 10, 2024

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

#102 (comment)

@0x4007
Copy link
Member Author

0x4007 commented Oct 10, 2024

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
Copy link
Member

Keyrxng commented Oct 20, 2024

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
Copy link
Member

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
Copy link
Member

Keyrxng commented Oct 31, 2024

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
Copy link

zugdev commented Oct 31, 2024

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
Copy link
Member Author

0x4007 commented Oct 31, 2024

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.

@0x4007 0x4007 closed this as not planned Won't fix, can't repro, duplicate, stale Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants