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

Refactor/remove supabase #13

Merged

Conversation

Keyrxng
Copy link
Contributor

@Keyrxng Keyrxng commented Jul 28, 2024

Resolves #12
May Resolve #10

  • removes Env and adapters
  • removes supabase completely
  • parses the deadline using two comments: the /start response string literal and the follow up comment created_at
  • adds ubiquibot-logger
  • new config item watch: {optIn, optOut}
  • removed the handlers as they only served supabase functionality

Which events should be defined in Supported Events the same as before and just run the plugin on each or is the wildcard the real event that matters?

It would be handy for the /start plugin to log the assignee name in the metadata (I'll add in one of my open PRs) and the responses can be mapped to users more reliably

time.ts is not used in my implementation but if it's preferred/needed I can incorporate it

Copy link
Contributor

Unused dependencies (1)

Filename dependencies
package.json typebox-validators

Unused exports (2)

Filename exports
src/helpers/get-env.ts getGithubIssue
src/helpers/time.ts getTimeEstimate

Unused types (1)

Filename types
src/types/github-types.ts GetRepo

@0x4007
Copy link
Member

0x4007 commented Jul 29, 2024

I suppose this is sort of double dipping tasks, perhaps it makes sense to consolidate into a single task #10

src/run.ts Outdated Show resolved Hide resolved
src/helpers/get-env.ts Outdated Show resolved Hide resolved
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Jul 29, 2024

I suppose this is sort of double dipping tasks, perhaps it makes sense to consolidate into a single task #10

It's not guaranteed that this does resolve #10 and removing supabase was necessary anyway wasn't it? I feel they should be kept separate

src/helpers/get-env.ts Outdated Show resolved Hide resolved
src/helpers/get-env.ts Outdated Show resolved Hide resolved
src/worker.ts Outdated Show resolved Hide resolved
src/helpers/get-env.ts Outdated Show resolved Hide resolved
src/helpers/get-env.ts Outdated Show resolved Hide resolved
Comment on lines 61 to 66
const botComments = comments.filter((o) => o.user?.type === "Bot");
const dateRegex = /(?<=<td>)(\w{3}, \w{3} \d{1,2}, \d{1,2}:\d{2} (AM|PM) UTC)(?=<\/td>)/gi;
const assignmentRegex = /Ubiquity - Assignment - start -/gi;
const botAssignmentComments = sortAndReturn(botComments.filter((o) => assignmentRegex.test(o?.body || "")), "desc");
const botFollowup = /this task has been idle for a while. Please provide an update./gi;
const botFollowupComments = botComments.filter((o) => botFollowup.test(o?.body || ""));
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not a good idea to rely on matching exact text because they might change. It'd be better if we store data in the comment metadata but I'm not sure if we have this functionality implemented.

Copy link
Member

@0x4007 0x4007 Jul 31, 2024

Choose a reason for hiding this comment

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

The part that was generally available/implemented was the metadata header which could be used as a selector to find the right comment:

I had a generalized function for writing metadata that would add a "comment header" which included the developer defined "metadata class name" and the function caller's name. This was intended to be used as a target for these situations.

https://github.com/ubiquity/ubiquibot/blob/development/src/handlers/shared/structured-metadata.ts#L9

I have successfully written search queries using the GitHub search which leveraged this.

https://t.me/UbiquityDAO/1/32137

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what you are seeing here is it not?

const assignmentRegex = /Ubiquity - Assignment - start -/gi;
<!-- Ubiquity - Assignment - start - 86f94c4
{
  "duration": 14400,
  "priceLabel": {
    "id": 7036610424,
    "node_id": "LA_kwDOL-In888AAAABo2oneA",
    "url": "https://api.github.com/repos/ubiquibot/command-start-stop/labels/Price:%20200%20USD",
    "name": "Price: 200 USD",
    "color": "1f883d",
    "default": false,
    "description": null
  }
}
-->

ubiquity-os-marketplace/command-start-stop#2 (comment)

It'd be better if we store data in the comment metadata but I'm not sure if we have this functionality implemented.

I forgot to last night but I will add that minor change into one of my open PRs and include the deadline, the userID etc and it can be mapped easier

Copy link
Member

Choose a reason for hiding this comment

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

I see, this metadata is generated by another plugin, right? Ideally it should be structured like JSON or something similar.
However my point still stands for bot follow-up regex but this can be fixed in a later PR

Copy link
Member

Choose a reason for hiding this comment

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

I just saw this thread, I had the same concern myself
#13 (comment)

Even by using metadata checking for a string seems unreliable and easily breakable. What is the purpose of checking for this bot comment?

src/helpers/update-tasks.ts Outdated Show resolved Hide resolved
src/helpers/update-tasks.ts Show resolved Hide resolved
src/helpers/update-tasks.ts Outdated Show resolved Hide resolved
src/types/plugin-inputs.ts Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Jul 31, 2024

I suppose this is sort of double dipping tasks, perhaps it makes sense to consolidate into a single task #10

It's not guaranteed that this does resolve #10 and removing supabase was necessary anyway wasn't it? I feel they should be kept separate

I don't know enough about the problem to say definitively either way.

@Keyrxng Keyrxng mentioned this pull request Aug 5, 2024
@Keyrxng
Copy link
Contributor Author

Keyrxng commented Aug 7, 2024

I didn't want to mess around with collect-linked-pulls.ts as it seems like quite a bit of work may have went into it, it felt rude almost to just remove it and it works so no need really.

I said to whilefoo that the search.issuesAndPullRequests is very OP earlier and it looks like it can be used to track linked PRs. I haven't tested it but I think nearly all codebases that require linkedPR tracking each has a unique way of doing it, maybe this the search endpoint is the simplest and most reliable? Thoughts on that?

@0x4007
Copy link
Member

0x4007 commented Aug 7, 2024

I didn't want to mess around with collect-linked-pulls.ts as it seems like quite a bit of work may have went into it, it felt rude almost to just remove it and it works so no need really.

The link doesn't work for me but the file name sounds familiar. Im pretty sure I authored it and it's fine to delete obsolete code.

I said to whilefoo that the search.issuesAndPullRequests is very OP earlier and it looks like it can be used to track linked PRs. I haven't tested it but I think nearly all codebases that require linkedPR tracking each has a unique way of doing it, maybe this the search endpoint is the simplest and most reliable? Thoughts on that?

If it is more reliable then we should switch to this approach.

@whilefoo
Copy link
Member

whilefoo commented Aug 7, 2024

I said to whilefoo that the search.issuesAndPullRequests is very OP earlier and it looks like it can be used to track linked PRs.

I can see that you can filter PRs that have linked issues, but how can you specify that you only want PRs that are linked to a particular issue?

@Keyrxng
Copy link
Contributor Author

Keyrxng commented Aug 7, 2024

I said to whilefoo that the search.issuesAndPullRequests is very OP earlier and it looks like it can be used to track linked PRs.

I can see that you can filter PRs that have linked issues, but how can you specify that you only want PRs that are linked to a particular issue?

repo:ubiquibot/user-activity-watcher is:open linked:issue "resolves #12" works fine. It is also case-insensitive regarding Resolves after manually testing via the UI. Some approaches consider other prefixes, idk how accurate it would be if we actually used the other regex captures but.

Thoughts?

@gentlementlegen
Copy link
Member

@Keyrxng I also like using the search api because it is powerful and reliable. If it works as expected and gives the same results, it would be great to replace the current code base with it because that wound greatly reduce the code base thus lowering the amount of possible bugs.

@whilefoo
Copy link
Member

repo:ubiquibot/user-activity-watcher is:open linked:issue "resolves #12" works fine. It is also case-insensitive regarding Resolves after manually testing via the UI. Some approaches consider other prefixes, idk how accurate it would be if we actually used the other regex captures but.

Thoughts?

I don't think this will work if the PR was linked via UI, however this already doesn't work with the current approach anyway because even though it relies on events, it still checks the comment. So I guess using search is still a better approach.

"resolves #12" will match any comment so we should limit to the PR description with in:body.
It would be also good to match other keywords. Github doesn't have any docs on boolean operators but it seems to work using OR: is:pr is:open in:body "closes #14" OR "resolves #14"

@gentlementlegen gentlementlegen mentioned this pull request Aug 23, 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 this, I guess it will be way more performant once we get #15 in.

@0x4007 0x4007 merged commit bb2c570 into ubiquity-os-marketplace:development Aug 24, 2024
2 checks passed
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.

Proposal: remove supabase Infinite trigger of the workflow
4 participants