Revert "fix(scheduling): query "/" to check if a runner is ready"#193
Revert "fix(scheduling): query "/" to check if a runner is ready"#193doringeman merged 1 commit intomainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR reverts the readiness probe change to query "/" and restores querying the "/v1/models" endpoint by updating the HTTP request URL. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @doringeman, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue by reverting a previous change that modified the readiness check mechanism for runners. The revert restores the original endpoint used to determine if a runner is operational, ensuring stability and correct behavior of the scheduling system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR reverts a previous change that modified the endpoint used for checking runner readiness from /v1/models back to /. This revert undoes the fix that was implemented in PR #170.
- Reverts the readiness check endpoint from "/" back to "/v1/models"
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -205,7 +205,7 @@ func (r *runner) wait(ctx context.Context) error { | |||
| default: | |||
| } | |||
| // Create and execute a request targeting a known-valid endpoint. | |||
There was a problem hiding this comment.
Reverting to /v1/models endpoint may reintroduce the original issue that PR #170 was intended to fix. Consider documenting why this revert is necessary and whether the underlying problem has been resolved through other means.
| // Create and execute a request targeting a known-valid endpoint. | |
| // Create and execute a request targeting a known-valid endpoint. | |
| // NOTE: This readiness check uses the `/v1/models` endpoint. | |
| // This was reverted to `/v1/models` following PR #170, which originally changed the endpoint | |
| // to address [describe the issue, e.g., a bug, security, or compatibility problem]. | |
| // The underlying problem has been [resolved/mitigated] through [other means/changes], so reverting is safe. | |
| // If the issue resurfaces, consider updating this endpoint and reviewing PR #170 for context. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts a previous change that set the runner readiness check endpoint to /. As the root endpoint handler returns a 404, that change was likely causing readiness checks to fail. This revert restores the endpoint to /v1/models, which is a standard OpenAI API endpoint for listing models and is a more suitable target for a readiness probe. The change is correct and improves the reliability of the runner. I have one minor suggestion to improve maintainability.
| } | ||
| // Create and execute a request targeting a known-valid endpoint. | ||
| readyRequest, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://localhost/", http.NoBody) | ||
| readyRequest, err := http.NewRequestWithContext(ctx, http.MethodGet, "http://localhost/v1/models", http.NoBody) |
There was a problem hiding this comment.
For better maintainability and to avoid magic strings, it's a good practice to define this URL as a constant. For example, you could declare const readinessProbeURL = "http://localhost/v1/models" at the package level and use it here.
Reverts #170
Summary by Sourcery
Bug Fixes: