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

/stop can start again #69

Open
0x4007 opened this issue Oct 27, 2024 · 47 comments
Open

/stop can start again #69

0x4007 opened this issue Oct 27, 2024 · 47 comments

Comments

@0x4007
Copy link
Member

0x4007 commented Oct 27, 2024

example

ubiquity/ts-template#61 (comment)

Calling stop should not penalize. If they called stop they should be able to reassign in the future becauss its not a disqualification.

@ishowvel
Copy link

the specification seems pretty straight forward

@ishowvel
Copy link

/start

Copy link

Deadline Mon, Oct 28, 8:08 AM UTC
Beneficiary 0x340D8d2bd82dEb4f485623453c9F6ad307e6B027

Tip

  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@ishowvel
Copy link

i have noticed that only 1 plugin works in the kernel at the time, my pricing plugin doesnt work when i add this start stop plugin to the .ubiquity-os config, the kernel only forwards the payload to one plugin for some reason

@ishowvel
Copy link

this may already be implemented

@ishowvel
Copy link

/stop

@ishowvel
Copy link

/start

Copy link

! ishowvel you were previously unassigned from this task. You cannot be reassigned.

@ishowvel
Copy link

@gentlementlegen might you know why the kernel is only forwarding payload to the first plugin found in any repo containing the ubiquity-os config like found here

[wrangler:inf] POST /events 200 OK (1397ms)
Creating empty kv KvNamespace {}
Event issue_comment received (id: bae1fd80-9536-11ef-8375-aa28c5354825)
Skipping plugin chain Assistive pricing because command does not match. {
  allow: {
    'ubiquity:example': '/allow @user1 label',
    description: 'Allows the user to modify the given label.'
  }
}
[wrangler:inf] POST /events 200 OK (1009ms)

@ishowvel
Copy link

the kernel doesnt even know if these plugins exist

@0x4007
Copy link
Member Author

0x4007 commented Oct 28, 2024

My best guess is that you have a formatting problem in your yml config. Sometimes it's difficult to get the indentations perfectly.

@gentlementlegen
Copy link
Member

It is trying to forward it but what it received is not a valid command like /label. Plugins react only to certain events / inputs.

@ishowvel
Copy link

Shouldn't it receive the valid commands for this plugin ie /start, /stop

@gentlementlegen
Copy link
Member

gentlementlegen commented Oct 28, 2024

In the configuration file you gave, you chained plugins so it won't. You need to change it to something like

plugins:
  - uses:
    - plugin: http://127.0.0.1:4000
      with:
        labels:
          time:
            - "Time: <15 Minutes"
            - "Time: <1 Hour"
            - "Time: <2 Hours"
            - "Time: <4 Hours"
            - "Time: <1 Day"
            - "Time: <1 Week"
            - "Time: <2 Weeks"
            - "Time: <1 Month"
          priority:
            - "Priority: 1 (Normal)"
            - "Priority: 2 (Medium)"
            - "Priority: 3 (High)"
            - "Priority: 4 (Urgent)"
            - "Priority: 5 (Emergency)"
        basePriceMultiplier: 2
        publicAccessControl:
          setLabel: true
          fundExternalClosedIssue: false
  -  uses:
    - plugin: ishowvel/daemon-disqualifier:compute.yml@testing
      with:
        disqualification: "2 minutes"
        warning: "1 minutes"
        watch:
          optOut:
            - "repoName"
            - "repoName2"
        eventWhitelist:
          - "review_requested"
          - "ready_for_review"
          - "commented"
          - "committed"
  - uses:
    - plugin: http://localhost:4001 # or the URL where the plugin is hosted
      name: start-stop
      id: start-stop-command
      with:
        reviewDelayTolerance: "3 Days"
        taskStaleTimeoutDuration: "30 Days"
        maxConcurrentTasks: # Default concurrent task limits per role.
          member: 5
          contributor: 3
        startRequiresWallet: true # default is true
        emptyWalletText: "Please set your wallet address with the /wallet command first and try again."
        rolesWithReviewAuthority: ["MEMBER", "OWNER"]
    

so every plugin is called individually.

@ishowvel
Copy link

@0x4007 can i be assigned to this

Copy link

@ishowvel the deadline is at Thu, Oct 31, 7:54 PM UTC

Copy link

A new workroom has been created for this task. Join chat

@ishowvel
Copy link

ishowvel commented Nov 1, 2024

@0x4007 i cant seem to recreate the issue

@ishowvel
Copy link

ishowvel commented Nov 1, 2024

the problem for me was i was using the github app id as the BOT_USER_ID but instead i should have used
image

@0x4007
Copy link
Member Author

0x4007 commented Nov 1, 2024

Show me the conversation/QA on GitHub so I have context on your problem

@ishowvel
Copy link

ishowvel commented Nov 2, 2024

ishowvel-org/.ubiquity-os#4
After a certain point i switched the BOT_USER_ID from the GitHub app Id to the GitHub app user id

@0x4007
Copy link
Member Author

0x4007 commented Nov 2, 2024

It looks like it works. @gentlementlegen rfc

@gentlementlegen
Copy link
Member

It should not, otherwise it means that users can also assign themselves again after disqualification.

@ishowvel
Copy link

ishowvel commented Nov 3, 2024

No I mean that if no code was changed, it should not allow re-assign with the current implementation.

if thats the case i dont see how this issue should even be worked on, i think this should be closed as not planned

@ishowvel
Copy link

ishowvel commented Nov 3, 2024

/stop

@ariesgun
Copy link
Contributor

ariesgun commented Nov 5, 2024

/start

@ariesgun
Copy link
Contributor

ariesgun commented Nov 6, 2024

Could not reproduce the issue in the local environment.
One possible reason it failed is BOT_USER_ID's misconfigured??

const botUnassigned = unassignedEvents.filter((event) => event.actorId === BOT_USER_ID);

@gentlementlegen
Copy link
Member

gentlementlegen commented Nov 8, 2024

The BOT_USER_ID should match the id of the app your installed in your org @Keyrxng rfc

This is still relevant since this line is in the codebase:

if (await hasUserBeenUnassigned(context, username)) {

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 8, 2024

BOT_USER_ID === payload.user.type === "BOT" && payload.user.login === "UbiquityOS"

It is the literal ID of the User entity which belongs to the app. Not the APP_ID which is the app's identifier.

rest.user.getByUsername("ubiquity-os-beta").data.id

@ariesgun
Copy link
Contributor

ariesgun commented Nov 8, 2024

/stop

@ariesgun
Copy link
Contributor

ariesgun commented Nov 8, 2024

/start

Copy link

! ariesgun you were previously unassigned from this task. You cannot be reassigned.

@ariesgun
Copy link
Contributor

ariesgun commented Nov 8, 2024

Cannot assign the issue to me anymore.

BOT_USER_ID === payload.user.type === "BOT" && payload.user.login === "UbiquityOS"

Not sure if this is what is used in the code, but shouldn't it be

rest.user.getByUsername("ubiquity-os-beta[bot]").data.id

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 8, 2024

Cannot assign the issue to me anymore.

BOT_USER_ID === payload.user.type === "BOT" && payload.user.login === "UbiquityOS"

Not sure if this is what is used in the code, but shouldn't it be

rest.user.getByUsername("ubiquity-os-beta[bot]").data.id

Yeah sorry you are correct it's probably better to use -beta[bot] than just ubiquity-os[bot] but you get the idea it's user.id of the bot entity

@ariesgun
Copy link
Contributor

ariesgun commented Nov 8, 2024

Cannot assign the issue to me anymore.

BOT_USER_ID === payload.user.type === "BOT" && payload.user.login === "UbiquityOS"

Not sure if this is what is used in the code, but shouldn't it be

rest.user.getByUsername("ubiquity-os-beta[bot]").data.id

Yeah sorry you are correct it's probably better to use -beta[bot] than just ubiquity-os[bot] but you get the idea it's user.id of the bot entity

I see.. so for this issue, is it possible to verify if the BOT_USER_ID in dev.vars is set properly or not?
If I look inside function,

if (await hasUserBeenUnassigned(context, username)) {
,

the most probable root cause is the event.actorId !== BOT_USER_ID

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 8, 2024

The cause is likely because we have two bots and so we have two BOT_USER_IDs to compare. Perhaps it makes sense to add both bot ids and check the actor.id against both?

That is just an assumption obviously but to answer the question: TypeBox verifies that the env vars are defined. Whether they reflect our actual bot is another question, we opt'd for the ID over the username a while back although if there is no other clean solution it'll have to do.

@ariesgun
Copy link
Contributor

ariesgun commented Nov 8, 2024

Or just check for [bot] in the actorname?

image

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 8, 2024

Or just check for [bot] for the actorname?

image

That's a quick fix I guess yes.

The original intention behind specifying it's our bot was so that in a partner org where they may have lots of little bots installed that perform actions on tasks and PRs this avoids any interference with our logic.

@gentlementlegen
Copy link
Member

I think it is safe to assume that a bot un-assign action is from our bot, and would cover both cases.

@ariesgun
Copy link
Contributor

ariesgun commented Nov 9, 2024

If it is agreed to do it this way, (check name for [bot] I can make the changes quickly .
Can you assign this task to me again?

@Keyrxng
Copy link
Contributor

Keyrxng commented Nov 9, 2024

@ariesgun Sounds to me like it was implied so yeah go ahead with that solution. I can't assign you but you can link the PR and you'll be assigned when someone else sees this

@gentlementlegen
Copy link
Member

@ariesgun i think you should check for the user type to be a bot instead.

Copy link

! This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: Priority: 3 (High), Priority: 4 (Urgent), Priority: 5 (Emergency)

@ariesgun
Copy link
Contributor

@0x4007 , is it possible to start this task?

Copy link

! This task does not reflect a business priority at the moment. You may start tasks with one of the following labels: Priority: 3 (High), Priority: 4 (Urgent), Priority: 5 (Emergency)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants