Skip to content

Conversation

@Cali0707
Copy link
Contributor

@Cali0707 Cali0707 commented Nov 5, 2025

Resolves #27

Signed-off-by: Calum Murray <cmurray@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Nov 5, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Cali0707
Copy link
Contributor Author

Cali0707 commented Nov 5, 2025

It's very early, but @manusa @matzew @jrangelramos does this API make sense?

The idea is that the env is tied to the MCP server, so we can have setup/teardown around a mcp server

For example, in kubernetes-mcp-server, this could create a KinD cluster + configure the mcp server with that kubeconfig file

@manusa
Copy link
Member

manusa commented Nov 6, 2025

Overall looks good.

Some nits and comments:

  • nit: I think tearDown instead of cleanup might be more intuitive as it's understood as the opposite of setup.
    Other options that are more clear and broader in scope could be beforeAll/afterAll and beforeEach/afterEach.
    • As example use cases:
      • beforeAll/afterAll could set up and tear down the kind cluster
      • beforeEach/afterEach could create a namespace or clean the namespace before and after each task.
  • Is the healthcheck supposed to be tested for each task execution? or is it just something to be performed once?
    Depending on its purpose, I think it's too narrow as it is.
    If it's just checking an endpoint why not just check if the MCP server is available?

I'm also not completely following on the managed http type (#27 (comment)).
If I'm following correctly we'd have 3 alternatives:

  • stdio: gevals launches the server in stdio mode
  • http: gevals has just an http server config and tries to connect to a server that's spawned separately
  • managedhttp: gevals launches the server in http mode? if so can't this case just be covered by the beforeAll or setup lifecycle hooks?

@Cali0707
Copy link
Contributor Author

Cali0707 commented Nov 7, 2025

managedhttp: gevals launches the server in http mode? if so can't this case just be covered by the beforeAll or setup lifecycle hooks?

@manusa my thinking here is that if we put it in the setup/teardown lifecycle hooks (which currently just run bash), it would end up having some bash command to run the server in a separate process and save the pid somewhere, which would then need a lookup and kill in the teardown. I thought that by giving the framework the info on how to run the server directly, it would be "nicer" as then gevals can manage running and shutting down the server directly.

IMO, the lifecycle should maybe be renamed to enviornmentLifecycle, as that is more representative of what it is doing

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.

Add eval setup/teardown support

2 participants