fix/[v1.30] Update Network API compatibility enhancement #1187
Conversation
📝 WalkthroughWalkthroughAdds two network code samples, clarifies the Changes
Sequence Diagram(s)(omitted — changes are examples, docstring edits, and tests without new multi-component control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
meilisearch/client.py (1)
900-922: Clarify parameter documentation for the Network HA configuration.The updated docstring improves clarity, but the parameter documentation could be more precise:
- The description states "must contain either" but the type signature allows
Noneas a valid value. Consider documenting whenNoneis appropriate.- The mutual exclusivity of 'remotes' vs. 'leader' isn't entirely clear. Based on the code sample in
.code-samples.meilisearch.yaml(lines 10-17), both fields can be present simultaneously (e.g.,{"remotes": {...}, "leader": None}).- Consider clarifying the use cases:
- Setting
"remotes"with"leader": Noneconfigures this instance as a Leader with specified Followers- Setting
"leader"with a URL configures this instance as a Follower pointing to the LeaderSuggested documentation improvement
def add_or_update_networks(self, body: Union[MutableMapping[str, Any], None]) -> Dict[str, str]: """Configure the Network High Availability (Leader/Followers). Parameters ---------- body: - The network configuration dictionary. must contain either: - - 'remotes': A dictionary of follower instances (Star Topology). - - 'leader': A string URL of the leader instance. + The network configuration dictionary: + - For Leader configuration: Set 'remotes' to a dictionary of follower instances and 'leader' to None. + - For Follower configuration: Set 'leader' to the Leader's URL string. + - Pass None to retrieve current configuration without changes. Returns.code-samples.meilisearch.yaml (1)
9-17: Consider using a placeholder instead of "masterKey" in the code sample.The code sample uses
"masterKey"as thesearchApiKeyvalue. While this may work in local development, it could encourage insecure practices if users copy this directly to production environments.Suggested improvement
update_network_1: |- client.add_or_update_networks({ "remotes": { "http://localhost:7700": { - "searchApiKey": "masterKey" + "searchApiKey": "your_search_api_key_here" } }, "leader": None })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.code-samples.meilisearch.yamlmeilisearch/client.pytests/client/test_client_network.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/client/test_client_network.py (2)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(900-922)
🪛 GitHub Actions: Tests
tests/client/test_client_network.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted, 75 files would be left unchanged. Run 'pipenv run black meilisearch tests --check' to format the code.
orbin123
left a comment
orbin123
left a comment
There was a problem hiding this comment.
style: fix black formatting
orbin123
left a comment
orbin123
left a comment
There was a problem hiding this comment.
passing black millisearch tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/client/test_client_network.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/client/test_client_network.py (3)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(900-922)tests/test_utils.py (1)
reset_network_config(37-38)
🪛 GitHub Actions: Tests
tests/client/test_client_network.py
[error] 1-1: pipenv run black meilisearch tests --check failed. 1 file would be reformatted (tests/client/test_client_network.py). Run 'pipenv run black' to fix code style issues.
orbin123
left a comment
orbin123
left a comment
There was a problem hiding this comment.
black formatting resolved
…n123/meilisearch-python into feat/1185-update-network-api-compat
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/client/test_client_network.py (1)
42-60: Good progress addressing previous feedback; use consistent parameter style.The test successfully implements the previously requested improvements (cleanup,
isinstancecheck, leader assertion). However, there's a style inconsistency at line 54.Style inconsistency: Line 54 uses
client.add_or_update_networks(body)(positional argument), while line 32 intest_add_or_update_networksusesclient.add_or_update_networks(body=body)(keyword argument). For consistency within the test file, prefer the keyword argument style.Optional enhancement: Consider verifying the remote's structure to make assertions more robust, as suggested in previous reviews.
🔎 Suggested improvements
- response = client.add_or_update_networks(body) + response = client.add_or_update_networks(body=body) assert isinstance(response, dict) assert REMOTE_MS_1 in response["remotes"] + assert response["remotes"][REMOTE_MS_1]["url"] == "http://localhost:7701" + assert response["remotes"][REMOTE_MS_1]["searchApiKey"] == "remoteSearchKey" assert response.get("leader") is None
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/client/test_client_network.pytests/settings/test_settings_embedders.py
✅ Files skipped from review due to trivial changes (1)
- tests/settings/test_settings_embedders.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/client/test_client_network.py (3)
tests/conftest.py (1)
client(15-16)meilisearch/client.py (1)
add_or_update_networks(900-922)tests/test_utils.py (1)
reset_network_config(37-38)
There was a problem hiding this comment.
hey @orbin123 thanks for this contribution!
Can you double-check all the documentation, please? I suggested some changes.
It looks like it's AI-generated because it doesn't match the feature documentation. It's better to be succinct than misleading.
Co-authored-by: Laurent Cazanove <lau.cazanove@gmail.com>
Co-authored-by: Laurent Cazanove <lau.cazanove@gmail.com>
|
Sir, I have commited the necessary documentation changes. thankyou |
2 participants
Pull Request
Related issue
Fixes #1185
What does this PR do?
This PR updates the SDK to be compatible with the Meilisearch v1.30 Network API (High Availability), which introduced breaking changes by removing the deprecated
shardingconfiguration in favor of a Leader/Follower topology.Changes:
client.add_or_update_networksandclient.get_all_networksdocumentation to reflect the newremotesandleaderpayload structure, removing obsolete references tosharding.get_network_1andupdate_network_1examples to .code-samples.meilisearch.yaml demonstrating the new API usage.sharding: truetests which are now invalid.test_configure_as_leaderto verify configuring the client as a Leader with Follower remotes.Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.