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

Use BOT_USER_ID over APP_ID #35

Merged

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Sep 3, 2024

I'm not sure that this is related to the previously linked issue so I'm removing the reference

  • APP_ID cannot be used to ID the bot
  • remove two comment postings
  • pass decodedEnv

@Keyrxng Keyrxng changed the title Use something other than APP_ID for botId Use BOT_USER_ID over APP_ID Sep 3, 2024
@0x4007
Copy link
Member

0x4007 commented Sep 3, 2024

What exactly is "bot user id"

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 3, 2024

What exactly is "bot user id"

It's literally the user.id of the bot entity. The id you'd get in the response if you used the fetchByUsername endpoint with the bot username

@Keyrxng Keyrxng mentioned this pull request Sep 7, 2024
Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the code changes although I am not sure to understand why it is needed? We'll have to add this variable to the environment once merged.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 8, 2024

I am fine with the code changes although I am not sure to understand why it is needed? We'll have to add this variable to the environment once merged.

It's required so that we are able to identify our bot unassigning contributors from the timeline and conversation and is used here

@0x4007
Copy link
Member

0x4007 commented Sep 8, 2024

Can somebody else corroborate this? I've never heard of this concept and don't think it's a necessary change. @whilefoo rfc

Besides without a linked specification I don't see why this is an authorized project.

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 8, 2024

Can somebody else corroborate this? I've never heard of this concept and don't think it's a necessary change. @whilefoo rfc

Besides without a linked specification I don't see why this is an authorized project.

#19 introduced here and the at the bottom of the conversation is where it's discussed. I first used the bot username and it was decided to use APP_ID but that does not correlate to the actual user.id of the bot.

@0x4007
Copy link
Member

0x4007 commented Sep 8, 2024

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Sep 8, 2024

The confusion is that a github app does not have a user.id, it has an APP_ID but the bot in which that app uses to interact with github is a real user entity with type: Bot and so it does have it's own user.id.

Deleting devpool issues is where I first noticed it, that pr and I wrote more about it there.

image

@0x4007
Copy link
Member

0x4007 commented Sep 8, 2024

If the following conditions are true we can merge this:

  • APP_ID doesn't work for our purposes
  • Bot user id is the same on two different organization installations.

@whilefoo
Copy link

whilefoo commented Sep 8, 2024

When fetching comments we do get information about the Github App which includes the APP ID, but in this case we are fetching events which don't include this for some reason.

"performed_via_github_app": {
      "id": 975031,
      "client_id": "Iv23ctBXjJSXqFzuoxRU",
      "slug": "ubiquity-os",
      "node_id": "A_kwHOCYyrXM4ADuC3",
      "owner": {
        "login": "Ubiquity-OS",
        "id": 160213852,
        "node_id": "O_kgDOCYyrXA",
        "avatar_url": "https://avatars.githubusercontent.com/u/160213852?v=4",
        "gravatar_id": "",
        "url": "https://api.github.com/users/Ubiquity-OS",
        "html_url": "https://github.com/Ubiquity-OS",
        "followers_url": "https://api.github.com/users/Ubiquity-OS/followers",
        "following_url": "https://api.github.com/users/Ubiquity-OS/following{/other_user}",
        "gists_url": "https://api.github.com/users/Ubiquity-OS/gists{/gist_id}",
        "starred_url": "https://api.github.com/users/Ubiquity-OS/starred{/owner}{/repo}",
        "subscriptions_url": "https://api.github.com/users/Ubiquity-OS/subscriptions",
        "organizations_url": "https://api.github.com/users/Ubiquity-OS/orgs",
        "repos_url": "https://api.github.com/users/Ubiquity-OS/repos",
        "events_url": "https://api.github.com/users/Ubiquity-OS/events{/privacy}",
        "received_events_url": "https://api.github.com/users/Ubiquity-OS/received_events",
        "type": "Organization",
        "site_admin": false
      },
      "name": "ubiquity-os",
      "description": "AI manager for remote teams",
      "external_url": "https://dao.ubq.fi/ubiquityos-for-daos",
      "html_url": "https://github.com/apps/ubiquity-os",
      "created_at": "2024-08-20T19:23:52Z",
      "updated_at": "2024-08-25T17:28:30Z"
}

I can confirm that bot user id is the same in ubiquibot and ubiquity organizations so I think we can merge this

src/handlers/shared/start.ts Show resolved Hide resolved
@@ -17,10 +17,8 @@ export class User extends Super {
if ((error && !data) || !data.wallets?.address) {
this.context.logger.error("No wallet address found", { userId, issueNumber });
if (this.context.config.startRequiresWallet) {
await addCommentToIssue(this.context, this.context.config.emptyWalletText);
await addCommentToIssue(this.context, this.context.logger.error(this.context.config.emptyWalletText, { userId, issueNumber }).logMessage.diff);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be better to use logger and throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean instead of posting a comment?

or you mean log const log = ..., addComment(log.logmessage.diff), throw new Error(log.raw)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean throw new Error(logger.error(...).diff), then top level catch will post it as a comment

@ubiquity-os ubiquity-os bot merged commit c13327c into ubiquity-os-marketplace:development Sep 11, 2024
2 checks passed
@whilefoo
Copy link

It doesn't make sense that the bot merged this if requested changes were not resolved yet

@0x4007
Copy link
Member

0x4007 commented Sep 12, 2024

It doesn't make sense that the bot merged this if requested changes were not resolved yet

Maybe we should require that all threads are resolved. But it seems that this rarely happens in practice, even internally with our own team.

I realize now that there was one approval, so this makes sense that it passed.

Requested changes (whilefoo) sometimes you mark that as the status and then forget to return so it makes sense.

@gentlementlegen
Copy link
Member

@whilefoo It was per design, but we can discuss this and change it eventually. I'd say pros are that it keeps PRs moving, cons is that if it is something important it should be taken care of during the review. On the other hand it can be fixed on a later PR as well.

0x4007 added a commit to 0x4007/command-start-stop that referenced this pull request Sep 14, 2024
@0x4007 0x4007 mentioned this pull request Sep 14, 2024
@0x4007
Copy link
Member

0x4007 commented Sep 14, 2024

Just reverted this pull and my Cloudflare Worker deploy works fine. What is the purpose of this pull?

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

Successfully merging this pull request may close these issues.

4 participants