-
Notifications
You must be signed in to change notification settings - Fork 78
feat: Add Actor runs API, dataset API, KV-store #122
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
Conversation
MQ37
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment otherwise LGTM 👍
|
@MQ37 This PR has grown quite a bit — sorry about that! Once again, we ended up working on the same part of the code, and when I started fixing the unit tests, I went down a rabbit hole and couldn’t stop myself. 😅 I really like the tests you added — they’re super helpful, thanks! That said, there’s one test I believe is tied to the reset function, which always loads the default Actor. So even when the server is called with a specific Actor selected, the default one still gets loaded — which shouldn’t happen. We’ll need to fix that. Also, I ran into an issue where, after the test completed, the process was still hanging around and consuming 100% CPU. If you could please review again. Mainly the test suite and mcp/server.ts 🙏🏻 |
MQ37
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor, thank you 👍 Just one issue with the Actorized MCP server tools listing otherwise LGTM
MQ37
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 We will fix the test reset func later
Closes: #79
Ideally, when using Actorized MCP servers we should add information about dataset and Actor run into context but for that we need to fix this issue first: apify/tester-mcp-client#67