Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Config/context overridden by another webhook #732

Open
whilefoo opened this issue Sep 7, 2023 · 32 comments · May be fixed by #801
Open

Config/context overridden by another webhook #732

whilefoo opened this issue Sep 7, 2023 · 32 comments · May be fixed by #801
Labels
Price: 600 USD Priority: 2 (Medium) Time: <1 Week

Comments

@whilefoo
Copy link
Collaborator

whilefoo commented Sep 7, 2023

I realized that bot config and context are global variables and when the bot runs and multiple webhooks are received at the same time, it will cause for one of them to override the other one. This is especially bad when the two webhooks are from two different repositories because a lot of functions use context to get repository and owner. Now because we are running the bot as Netlify function on production, as far as I know it runs the whole script as a separate process for every request so that avoid the problem. But if we run the bot as one process like during development then this will cause problems.

As I'm writing this I remember one case where an issue was created with 2 labels so Github sent 3 webhooks, one for issue opened and two for labels. The two webhooks for labels were exactly the same, which I thought was weird at first but in the end I thought this was Github's problem. Now it all makes sense!

Conclusion:
We should avoid using global variables, so context and bot config should be passed down by parameters which I know is more verbose but it's the only solution I can think of right now. If anyone has any suggestions, please comment.
@ubiquibot ubiquibot bot mentioned this issue Sep 7, 2023
Closed
@Keyrxng Keyrxng mentioned this issue Sep 18, 2023
Open
@Sadaf-A
Copy link
Contributor

Sadaf-A commented Sep 19, 2023

/start

@ubiquibot
Copy link

ubiquibot bot commented Sep 19, 2023

Deadline Tue, 19 Sep 2023 08:05:04 UTC
Registered Wallet 0x0BEd00438D57d07E3667b85Fa8EB86Af147C7025
Tips:
  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address @user.
  • 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 bounty.

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Sep 19, 2023 •
    edited
    Loading

    @whilefoo could you please link the file where we have the global variables and where we are using them?

    @whilefoo
    Copy link
    Collaborator Author

    whilefoo commented Sep 19, 2023 •
    edited
    Loading

    https://github.com/ubiquity/ubiquibot/blob/development/src/bindings/event.ts#L11-L16 botConfig and botContext, maybe we can we can create our own Context type that extends probot's Context and adds botConfig, that way we can pass only one argument (context) to functions instead of passing context and config.

    Also adapters are initialized on every event which doesn't make sense because they are not dependant on the event or config and always stay the same, so it would be better to initialize them only once when the bot starts up.

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Sep 22, 2023

    https://github.com/ubiquity/ubiquibot/blob/development/src/bindings/event.ts#L11-L16 botConfig and botContext, maybe we can we can create our own Context type that extends probot's Context and adds botConfig, that way we can pass only one argument (context) to functions instead of passing context and config.

    Also adapters are initialized on every event which doesn't make sense because they are not dependant on the event or config and always stay the same, so it would be better to initialize them only once when the bot starts up.

    export interface BotContext extends Context {
  botConfig: BotConfig;
}

    I made this interface and passed it as an argument in event.ts but I'm getting a lot of errors saying '"../../../bindings"' has no exported member 'getBotConfig' as I removed it. What do you think is the best solution to this?

    @whilefoo
    Copy link
    Collaborator Author

    Can you create PR even if it's failing, then I can help you out

    @Sadaf-A Sadaf-A linked a pull request Sep 23, 2023 that will close this issue
    Draft
    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Sep 23, 2023

    @whilefoo

    Can you create PR even if it's failing, then I can help you out

    done, thanks

    @ubiquibot
    Copy link

    ubiquibot bot commented Sep 27, 2023

    Do you have any updates @Sadaf-A? If you would like to release the bounty back to the DevPool, please comment /stop
    Last activity time: Sat Sep 23 2023 11:02:41 GMT+0000 (Coordinated Universal Time)

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Sep 27, 2023

    Do you have any updates @Sadaf-A? If you would like to release the bounty back to the DevPool, please comment /stop
    Last activity time: Sat Sep 23 2023 11:02:41 GMT+0000 (Coordinated Universal Time)

    @whilefoo is yet to reply to my comment I will move further omce that happens

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Oct 1, 2023

    /stop

    @ubiquibot ubiquibot bot unassigned Sadaf-A Oct 1, 2023
    @ubiquibot
    Copy link

    ubiquibot bot commented Oct 1, 2023

    You have been unassigned from the bounty @Sadaf-A

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Oct 1, 2023

    @whileoo please let me know when you are available till then I will try to work on something else!

    @Sadaf-A
    Copy link
    Contributor

    Sadaf-A commented Oct 5, 2023

    /start

    @ubiquibot
    Copy link

    ubiquibot bot commented Oct 5, 2023

    Deadline Thu, 05 Oct 2023 19:41:58 UTC
    Registered Wallet 0x0BEd00438D57d07E3667b85Fa8EB86Af147C7025
    Tips:
    • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address @user.
    • 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 bounty.

      @ubiquibot
      Copy link

      ubiquibot bot commented Oct 9, 2023

      Do you have any updates @Sadaf-A? If you would like to release the bounty back to the DevPool, please comment /stop
      Last activity time: Thu Oct 05 2023 18:41:54 GMT+0000 (Coordinated Universal Time)

      @ubiquibot ubiquibot bot unassigned Sadaf-A Oct 12, 2023
      @ubiquibot
      Copy link

      ubiquibot bot commented Oct 12, 2023

      @Sadaf-A - Releasing the bounty back to dev pool because the allocated duration already ended!
      Last activity time: Thu Oct 05 2023 18:41:54 GMT+0000 (Coordinated Universal Time)

      @whilefoo whilefoo mentioned this issue Oct 17, 2023
      Merged
      @ubiquibot ubiquibot bot removed the Price: 25 USD label Oct 18, 2023
      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 24, 2023

      /start

      @ubiquibot
      Copy link

      ubiquibot bot commented Oct 24, 2023

      Skipping /start because it is disabled on this repo

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 24, 2023

      /assign

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 24, 2023 •
      edited
      Loading

      @pavlovcik @whilefoo did something change in the bot how should i assign myself?
      I'm sorry for the delay I had a PR open for this but I had exams so I had to stop working on it. I will continue working on that PR now.

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 24, 2023

      /start

      @ubiquibot
      Copy link

      ubiquibot bot commented Oct 24, 2023

      Skipping /start because it is disabled on this repo

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 24, 2023

      @whilefoo @pavlovcik I've resolved all the errors in my PR could you please assign me. I don't know why the bot is not able to do so. Thank you.

      @0x4007
      Copy link
      Member

      0x4007 commented Oct 25, 2023

      This was addressed but @whilefoo on my refactor pull request. Because my pull request is addressing basically all the immediate needs of the bot, and because it's such a huge change to the codebase, I decided to disable public tasks in this repository until it's merged in which I expect to be next week sometime.

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Oct 25, 2023

      This was addressed but @whilefoo on my refactor pull request. Because my pull request is addressing basically all the immediate needs of the bot, and because it's such a huge change to the codebase, I decided to disable public tasks in this repository until it's merged in which I expect to be next week sometime.

      could you manually assign me for now?

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Nov 6, 2023

      /start

      @ubiquibot UbiquiBot
      Copy link

      ubiquibot bot commented Nov 6, 2023

      Skipping /start because it is disabled on this repo

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Nov 6, 2023

      This was addressed but @whilefoo on my refactor pull request. Because my pull request is addressing basically all the immediate needs of the bot, and because it's such a huge change to the codebase, I decided to disable public tasks in this repository until it's merged in which I expect to be next week sometime.

      hey any update?

      @korrrba-bot korrrba-bot bot mentioned this issue Nov 6, 2023
      Open
      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Nov 13, 2023

      /start

      @ubiquibot UbiquiBot
      Copy link

      ubiquibot bot commented Nov 13, 2023

      Skipping /start because it is disabled on this repo

      @Sadaf-A
      Copy link
      Contributor

      Sadaf-A commented Nov 29, 2023

      /start

      @ubiquibot UbiquiBot
      Copy link

      ubiquibot bot commented Nov 29, 2023

      Skipping /start because it is disabled on this repo

      Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
      Assignees
      No one assigned
      Labels
      Price: 600 USD Priority: 2 (Medium) Time: <1 Week
      Projects
      None yet
      Milestone
      No milestone
      Development

      Successfully merging a pull request may close this issue.

      Config/context overridden by another webhook Sadaf-A/ubiquibot
      3 participants
      @0x4007 @Sadaf-A @whilefoo