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

feat: task limit improvement #66

Conversation

ariesgun
Copy link
Contributor

Resolves #64

@ariesgun ariesgun marked this pull request as ready for review October 20, 2024 09:52
src/utils/issue.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Oct 20, 2024

You also should post QA

@ariesgun
Copy link
Contributor Author

You also should post QA

sorry, new here. what do you mean by "post QA"?

@0x4007
Copy link
Member

0x4007 commented Oct 20, 2024

Prove that it works on your org. This would be a link to a github comment

src/utils/issue.ts Outdated Show resolved Hide resolved
@ariesgun ariesgun requested a review from 0x4007 October 21, 2024 16:37
@ariesgun
Copy link
Contributor Author

Prove that it works on your org. This would be a link to a github comment

Test Result:

https://github.com/ubiquity-ariesgun/command-start-stop/issues/9#issuecomment-2427166082

src/plugin.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Oct 22, 2024

Test Result:

ubiquity-ariesgun#9 (comment)

Currently looking into adjusting the display format:


Warning

You have reached your max task limit. Please close out some tasks before assigning new ones.

- ###### [ubiquity-os-marketplace/command-start-stop - Task Limit Improvements #64](https://www.github.com/ubiquity-os-marketplace/command-start-stop/issues/64)

@ariesgun
Copy link
Contributor Author

QA:

https://github.com/ubiquity-ariesgun/command-start-stop/issues/12#issuecomment-2439593830

tests/main.test.ts Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Oct 27, 2024

Need to specifically handle 404

@ariesgun
Copy link
Contributor Author

Need to specifically handle 404

Any specific action when is 404?
Right now it will log and write it as a comment in github in case of non-200

@0x4007
Copy link
Member

0x4007 commented Oct 28, 2024

I wish we had a mechanism to detect if this breaks to respond quickly because that system may be changed without notice in the future. I suppose for now you should throw a specific and descriptive error if it 404.

A lot of work is being done on the devpool-directory backend so I anticipate that this file name will eventually be changed or removed so the faster we can diagnose this problem the better.

package.json Outdated Show resolved Hide resolved
ubiquity-ubiquibot

This comment was marked as resolved.

@gentlementlegen

This comment has been minimized.

Copy link
Member

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

forgot to submit review :D

src/handlers/shared/start.ts Outdated Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
@0x4007
Copy link
Member

0x4007 commented Oct 31, 2024

Should be configurable
Repo
Org
Network

Also that's wild that I can't delete or hide your review comment

@gentlementlegen
Copy link
Member

@0x4007 Why did you hide my comment (if that was you)?

@0x4007
Copy link
Member

0x4007 commented Nov 1, 2024

Your review comment is the exact same content

Copy link

@ariesgun, this task has been idle for a while. Please provide an update.

@gentlementlegen
Copy link
Member

@0x4007 maybe you are confused because I first posted my comment with the wrong account and then hid it to repost with my own? I can't seem to find twice the same comment from myself.

@0x4007
Copy link
Member

0x4007 commented Nov 2, 2024

image

@gentlementlegen
Copy link
Member

Yes that was with the wrong account, I hid that comment.

@ariesgun ariesgun force-pushed the task-limit-improvement branch from 316467b to 32e9280 Compare November 3, 2024 15:41
@ariesgun
Copy link
Contributor Author

ariesgun commented Nov 3, 2024

Made the issues search configurable: repo, org, or network.
Fixed the merge conflict.

Network:
https://github.com/ubiquity-ariesgun/command-start-stop/issues/12#issuecomment-2453487597

Org:
https://github.com/ubiquity-ariesgun/command-start-stop/issues/12#issuecomment-2453486116

Repo:
https://github.com/ubiquity-ariesgun/command-start-stop/issues/12#issuecomment-2453485977

@@ -16,6 +53,7 @@ export async function startStopTask(inputs: PluginInputs, env: Env) {
eventName: inputs.eventName,
payload: inputs.eventPayload,
config: inputs.settings,
organizations: [],
Copy link
Member

Choose a reason for hiding this comment

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

This should not be here but in the configuration.

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 the reasoning is that it's needed at multiple places so they just put it in the context

Copy link
Member

Choose a reason for hiding this comment

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

The spec states:

Only check assigned issues within the network, but this should be configurable to either be just within the repo, org, or network

Shouldn't this be configurable? And even if not, putting it in the config also allows to add defaults on decode which would ensure it is properly initialized and types as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It mentions the scope is configurable: repo, org , or network.. Dont understand what you meant

Copy link
Member

Choose a reason for hiding this comment

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

maybe I'm missing something, but the config item assignedIssueScope can be set to repo, org or network, this configuration property gets filled based on assignedIssueScope when the plugin starts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's how it behaves.. Based on assignedIssueScope, it will fill in the organization accordingly.

.paginate(context.octokit.rest.search.issuesAndPullRequests, {
q: `org:${payload.repository.owner.login} assignee:${username} is:open is:issue`,
q: `${repoOrgQuery} assignee:${username} is:open is:issue`,
Copy link
Member

Choose a reason for hiding this comment

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

Since you use the organizations with a search, it is possible that the user has a private profile which will result in a 422 error when querying the search API. You need to provide a fallback for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already a function getAssignedIssuesFallback isnt it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could reproduce it, but now i am not sure what kind of fallback we can do here??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm, the function getAssignedIssuesFallback handled it gracefully.

Copy link
Member

Choose a reason for hiding this comment

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

When I tested with a user having a private profile, the run crashed on a 422 error. I will test again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the construct to 'await' so it will be able to catch 422 and trigger getAssignedIssuesFallback

src/types/plugin-input.ts Outdated Show resolved Hide resolved
src/utils/issue.ts Outdated Show resolved Hide resolved
src/plugin.ts Outdated Show resolved Hide resolved
manifest.json Outdated Show resolved Hide resolved
@gentlementlegen
Copy link
Member

gentlementlegen commented Nov 6, 2024

Last QA looks ok: Meniole#46

However when using network option to scan for PRs, I could go beyond the limit of one assigned issue: Meniole#47

@ariesgun What does network look for?

@ariesgun
Copy link
Contributor Author

ariesgun commented Nov 6, 2024

Last QA looks ok: Meniole#46

However when using network option to scan for PRs, I could go beyond the limit of one assigned issue: Meniole#47

@ariesgun What does network look for?

it is gonna look at devpool-issues.json and look for any assigned issues there

@gentlementlegen gentlementlegen merged commit 6a013ba into ubiquity-os-marketplace:development Nov 8, 2024
2 checks passed
@ubiquity-os-beta ubiquity-os-beta bot mentioned this pull request Nov 9, 2024
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.

Task Limit Improvements
6 participants