feat(server): allow partial source startup checks#2662
feat(server): allow partial source startup checks#2662Deeven-Seru wants to merge 5 commits intogoogleapis:mainfrom
Conversation
Summary of ChangesHello, 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 significantly enhances the server's resilience and flexibility by allowing partial source availability at startup. It introduces mechanisms to bypass initial connectivity checks for data sources, either globally via a command-line flag or on a per-source basis through configuration. This prevents the server from failing to start due to transient network issues or unavailable dependencies, making it more robust in dynamic environments. Additionally, it includes a security improvement for HTTP tool URL handling. Highlights
Changelog
Activity
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.
Code Review
This pull request introduces a valuable feature to allow the server to start even if some data sources are unavailable, utilizing a new --allow-partial-sources flag and a per-source checkAtStartup configuration. While it includes a security hardening measure in the HTTP tool to prevent host and scheme overrides, the new URL construction logic remains vulnerable to path traversal attacks within the same host. This should be mitigated by validating the resolved path or properly escaping path parameters. Additionally, a suggestion is made to improve error handling in the valkey source to prevent fatal errors on connection failure, aligning it with the PR's goal.
2ffe70e to
35cfc27
Compare
|
Update: rebased this PR onto main to keep it focused on startup checks only. The HTTP tool host-override changes live in #2655, so the path-traversal comment is now out of scope here. Also updated valkey init to return an error instead of log.Fatalf, so allow-partial-sources can skip it cleanly. |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for allowing partial source availability during server startup, controlled by both a global flag and per-source configurations. The implementation is clean and robust, utilizing context to propagate settings and a centralized helper function for connectivity checks. The changes are consistently applied across all data sources, and the documentation has been updated accordingly. I appreciate that you also took the opportunity to fix some related issues, such as replacing log.Fatalf with proper error returns and using the correct context for ping operations. I have a couple of minor suggestions in internal/sources/redis/redis.go to improve error wrapping for consistency with other parts of the codebase.
35cfc27 to
6541422
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to allow partial source startup by skipping connectivity checks. This is controlled by a global --allow-partial-sources flag and a per-source checkAtStartup configuration option. The implementation correctly uses context to propagate the check setting and refactors all data sources to use a centralized CheckConnectivity helper, which is a great improvement. The documentation and tests have also been updated. My feedback includes a suggestion to enhance test coverage for the new configuration option and a potential refactoring to simplify how the static configuration is handled during dynamic reloads.
6541422 to
c87c8a2
Compare
|
Updates pushed:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to allow the server to start even if some data sources are unavailable. This is achieved by adding a global --allow-partial-sources flag and a per-source checkAtStartup configuration option. The core logic is implemented via a new CheckConnectivity helper in the internal/sources package, which is now used by all data source initialization methods to conditionally perform connectivity checks. The changes also include corresponding documentation updates and a new test for parsing the per-source configuration. The implementation is robust and consistently applied across all relevant parts of the codebase, and it also includes beneficial refactoring, such as replacing a log.Fatalf call with proper error handling in the Valkey source.
Note: Security Review did not run due to the size of the PR.
|
Thank you for the PR. We will review it shortly. |
9f38fa0 to
95b3ceb
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature to allow the server to start even if some data sources are unavailable, controlled by a new --allow-partial-sources flag and a per-source checkAtStartup option. The implementation is well-designed, centralizing the logic in internal/sources/sources.go and consistently applying it across all source initializations by refactoring them to use a new sources.CheckConnectivity helper. The changes are accompanied by documentation updates and a regression test. The overall implementation is solid and I have no comments for improvement.
Summary
checkAtStartupper-source config to skip startup connectivity checks--allow-partial-sourcesflag to skip all source checks at startupTesting
Fixes #2341