fix: resolve service discovery issues and enhance deployment#53
Conversation
- Copy bundled specs to cache on startup to ensure 270+ services are visible - Prevent failed or partial API list downloads from breaking discovery - Add environment metadata to infrastructure deployment payload
Summary of ChangesHello @StatPan, 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 improves the reliability and initialization of service discovery within the application. It addresses a critical bug that limited service visibility by implementing a new strategy to ensure the "all_apis.json" master list is correctly loaded, first by checking for a bundled version and then falling back to an API download. This change ensures that all available services are properly recognized and accessible from the start. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
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
|
![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 effectively addresses the service discovery issue by introducing a robust fallback mechanism using a bundled all_apis.json file, which enhances the application's resilience. The refactoring of the client map reloading logic into a helper function is also a good step towards cleaner code. I've provided a few suggestions to further improve maintainability and debuggability, focusing on replacing a magic number, restoring more detailed error logging, and preserving important contextual comments in docstrings.
| cache_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # 1. Check if master file already exists in cache | ||
| if master_file.exists() and master_file.stat().st_size > 1000: |
There was a problem hiding this comment.
The check master_file.stat().st_size > 1000 uses a magic number (1000). This makes the code harder to understand and maintain. It's better to define this value as a constant with a descriptive name. This clarifies the intent of the check (e.g., ensuring the file is not empty or truncated) and makes it easier to change if needed.
Please define MIN_MASTER_LIST_SIZE_BYTES = 1000 at the module level and use it here as suggested.
| if master_file.exists() and master_file.stat().st_size > 1000: | |
| if master_file.exists() and master_file.stat().st_size > MIN_MASTER_LIST_SIZE_BYTES: |
| if SERVICE_LIST_API_ID not in data: | ||
| if "RESULT" in data: | ||
| code = data["RESULT"].get("CODE") | ||
| msg = data["RESULT"].get("MESSAGE") | ||
| raise RuntimeError(f"Failed to fetch master list: {code} - {msg}") | ||
| raise RuntimeError(f"Invalid response format for master list: {data.keys()}") | ||
| raise RuntimeError(f"Invalid response format for master list") |
There was a problem hiding this comment.
The error handling for a failed API download has been simplified, but this removes potentially useful debugging information. The previous implementation extracted the error code and message from the API's RESULT field. Restoring this more detailed error reporting will make it easier to diagnose API-related issues during startup.
| if SERVICE_LIST_API_ID not in data: | |
| if "RESULT" in data: | |
| code = data["RESULT"].get("CODE") | |
| msg = data["RESULT"].get("MESSAGE") | |
| raise RuntimeError(f"Failed to fetch master list: {code} - {msg}") | |
| raise RuntimeError(f"Invalid response format for master list: {data.keys()}") | |
| raise RuntimeError(f"Invalid response format for master list") | |
| if SERVICE_LIST_API_ID not in data: | |
| if "RESULT" in data: | |
| code = data["RESULT"].get("CODE") | |
| msg = data["RESULT"].get("MESSAGE") | |
| raise RuntimeError(f"Failed to fetch master list: {code} - {msg}") | |
| raise RuntimeError(f"Invalid response format for master list. Keys: {list(data.keys())}") |
|
|
||
|
|
||
| def _reload_client_maps(client: AssemblyAPIClient, cache_dir: Path) -> None: | ||
| """Helper to reload client service maps from cache directory.""" |
There was a problem hiding this comment.
This new helper function is a good refactoring. However, the original code included an important comment explaining that modifying the client's internal attributes is a workaround because the assembly_client library lacks a public API for reloading specs. This context is valuable for future maintainers. It would be best to include this information in the docstring of _reload_client_maps.
| """Helper to reload client service maps from cache directory.""" | |
| """Helper to reload client service maps from cache directory. | |
| NOTE: This is a workaround that directly modifies internal attributes of the | |
| AssemblyAPIClient instance. It is necessary because the assembly_client library | |
| does not provide a public API to reload specs. If the library is updated, | |
| this should be replaced with a call to the official public method. | |
| """ |
1 participant
Changes
all_apis.jsonis now copied to the cache directory on startup.env: productionmetadata.