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

Handling issues.assigned for task-xp-guard #94

Open
Keyrxng opened this issue Aug 24, 2024 · 15 comments
Open

Handling issues.assigned for task-xp-guard #94

Keyrxng opened this issue Aug 24, 2024 · 15 comments

Comments

@Keyrxng
Copy link
Contributor

Keyrxng commented Aug 24, 2024

Related to https://github.com/ubiquibot/task-xp-guard/pull/1

The issue is that after using the /start command an issues.assigned event is fired but this line in the kernel skips the plugin because skipBotEvents defaults to true and the sender of that event is the bot which is valid in regards to issues.assigned/task-xp-guard.

Which is why I could only get it to run if I created a two-step chain passing the issue_comment.created event forward from start-stop to task-xp-guard

My quick workaround below but is there a better way to handle this sort of thing? I have tried using skipBotEvents: false on both plugins with no success. The only thing that seems to work for me is below.

 if (
    pluginChain.skipBotEvents && 
    "sender" in event.payload && 
    event.payload.sender?.type === "Bot" && 
    context.key !== "issues.assigned"
  ) {
    console.log("Skipping plugin chain because sender is a bot");
    return true;
  }
  • see this QA - shows it failing to catch the event despite skipBotEvents: false in the config

  • see this QA - shows that with my workaround it kicks me as it should not only via direct UI assignment

@0x4007
Copy link
Member

0x4007 commented Aug 25, 2024

@gentlementlegen @whilefoo rfc

@gentlementlegen
Copy link
Member

I had a similar issue when using automated-merging with conversation-rewards being skipped due to the bot nature of the action. I tried to use skipBotEvents but no task was closed by automation yet so I cannot confirm if it works or not. I think this is important to solve, which technically should be handled just by setting skipBotEvents to false.

@whilefoo
Copy link
Contributor

whilefoo commented Aug 26, 2024

@Keyrxng did you try console logging the skipBotEvents? because it should definitely not skip if it's set to false. I will try this myself too

@whilefoo
Copy link
Contributor

whilefoo commented Sep 5, 2024

@Keyrxng can you show me the config you used?

@whilefoo
Copy link
Contributor

whilefoo commented Sep 5, 2024

also I'm thinking that skipBotEvents should be in the manifest because the plugin developer should set it accordingly and not the partner

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 6, 2024

also I'm thinking that skipBotEvents should be in the manifest because the plugin developer should set it accordingly and not the partner

Agreed

can you show me the config you used?

I posted one here. But I tried a variety, the one posted just attempts to show my configuration in installing both:

  • 1 plugin which uses a plugin in it's own chain
  • 2 plugins which are separate from each other

I tried with skipBotEvents defined and undefined in both plugins and tried to cover all bases.

@whilefoo
Copy link
Contributor

whilefoo commented Sep 6, 2024

Because I'm wondering if you set skipBotEvents correctly on the plugin chain and not on the plugin itself

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 7, 2024

I think we could be doing with better naming conventions for plugin chaining lmao, just me that finds it slightly confusing when discussing chains?

Because I'm wondering if you set skipBotEvents correctly on the plugin chain and not on the plugin itself

I set skipBotEvents on each plugin but your comment reads like I should have placed it before - plugin and after - uses. Is that how it should be, where the comment is? I just tried this and the kernel complains

plugins:
  - uses:
    # skipBotEvents: false
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
      skipBotEvents: false # seems to have no effect whether true or false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
      skipBotEvents: false

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 7, 2024

This is the kernel logs running /start with task-xp-guard

image

This is an added log inside the conditional showing it hits on issues.assigned.

image

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 7, 2024

@gentlementlegen I can't find the discussion but it was mentioned about the base event such as issues being a valid event but the kernel not accepting it?

It shows in logs above that it's receiving the base event as a payload perhaps that has something to do with this problem @whilefoo?

@gentlementlegen
Copy link
Member

@Keyrxng the event logged does not represent the real event used, it is very likely issues.some_event

@whilefoo
Copy link
Contributor

whilefoo commented Sep 7, 2024

It should be like this:

plugins:
  - uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
    skipBotEvents: false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
    skipBotEvents: false

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 7, 2024

It should be like this:

plugins:
  - uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]
    skipBotEvents: false
  - uses:
    - plugin: http://localhost:4000
      runsOn:  ["issue_comment.created"]
    skipBotEvents: false

😓 So it does work with the config set like this. My bad, although it's weird syntax I think because skipBotEvents is an item of the - uses array not an item of the plugin array but if you place skipBotEvents before - plugin it doesn't work despite it still being in the same - uses block.

I think your idea to move this into the manifest is a better idea. I guess this will also be the solution for #98

@whilefoo
Copy link
Contributor

whilefoo commented Sep 8, 2024

skipBotEvents is outside uses even though it may not look like it
This is how you do it:

plugins:
  - skipBotEvents: false
    uses:
    - plugin: http://localhost:4001
      runsOn: [ "issues.assigned"]

but it is a bit confusing because there are two nested arrays

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 8, 2024

Thanks for explaining, it makes more sense seeing that.

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

No branches or pull requests

4 participants