fix/update_indexer_cookie_policy - #308
Merged
Merged
Conversation
… returning any cookie
Contributor
WalkthroughThe update primarily refines the handling of cookies within network communication, accommodating scenarios where cookies are not used by the Indexer exchange server but are required by the chain server. It introduces checks and error handling for cookie-related operations, ensuring robustness in network metadata management. Additionally, the removal of Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #308 +/- ##
==========================================
+ Coverage 73.06% 74.92% +1.85%
==========================================
Files 48 48
Lines 3416 3422 +6
Branches 304 306 +2
==========================================
+ Hits 2496 2564 +68
+ Misses 839 772 -67
- Partials 81 86 +5 ☔ View full report in Codecov by Sentry. |
nicolasbaum
reviewed
Feb 29, 2024
| expiration_time = 0 | ||
| else: | ||
| expiration_time = datetime.datetime.strptime( | ||
| cookie["GCLB"]["expires"], "%a, %d-%b-%Y %H:%M:%S %Z" |
There was a problem hiding this comment.
Is there any chance "GCLB" key is present, but "expires" sub-key is not?
Contributor
Author
There was a problem hiding this comment.
Not in the examples I checked. If that happens the expiration check should fail, since that would be unexpected.
There is no way to test it correctly now because there are no k8s servers supported
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.tomlis excluded by:!**/*.toml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- pyinjective/core/network.py (4 hunks)
- tests/core/test_network.py (1 hunks)
Additional comments: 14
tests/core/test_network.py (8)
- 6-18: The test
test_chain_metadatain theTestBareMetalLoadBalancedCookieAssistantclass correctly simulates the scenario where a cookie is present in the server response and asserts that the metadata is correctly formed. This test is well-structured and follows best practices for asynchronous testing with pytest.- 20-29: The test
test_chain_metadata_fails_when_cookie_info_not_included_in_server_responsein theTestBareMetalLoadBalancedCookieAssistantclass effectively simulates a failure scenario where the server response does not include cookie information. The use ofpytest.raisesto expect aRuntimeErroris appropriate and correctly implemented.- 31-42: The test
test_exchange_metadatain theTestBareMetalLoadBalancedCookieAssistantclass correctly simulates the scenario where a cookie is present in the server response for exchange metadata. The test is well-structured, and the assertion validates the expected behavior correctly.- 44-54: The test
test_exchange_metadata_is_none_when_cookie_info_not_included_in_server_responsein theTestBareMetalLoadBalancedCookieAssistantclass effectively tests the scenario where no cookie information is included in the server response for exchange metadata. The assertion thatmetadataisNoneis correct and aligns with the expected behavior.- 57-69: The test
test_chain_metadatain theTestKubernetesLoadBalancedCookieAssistantclass correctly simulates the scenario where a cookie is present in the server response and asserts that the metadata is correctly formed. This test mirrors the structure and logic of the corresponding test in theTestBareMetalLoadBalancedCookieAssistantclass, maintaining consistency across tests.- 71-80: The test
test_chain_metadata_fails_when_cookie_info_not_included_in_server_responsein theTestKubernetesLoadBalancedCookieAssistantclass effectively simulates a failure scenario where the server response does not include cookie information. The use ofpytest.raisesto expect aRuntimeErroris appropriate and correctly implemented, mirroring the structure of the corresponding test in theTestBareMetalLoadBalancedCookieAssistantclass.- 82-93: The test
test_exchange_metadatain theTestKubernetesLoadBalancedCookieAssistantclass correctly simulates the scenario where a cookie is present in the server response for exchange metadata. The test is well-structured, and the assertion validates the expected behavior correctly, mirroring the structure of the corresponding test in theTestBareMetalLoadBalancedCookieAssistantclass.- 95-105: The test
test_exchange_metadata_is_none_when_cookie_info_not_included_in_server_responsein theTestKubernetesLoadBalancedCookieAssistantclass effectively tests the scenario where no cookie information is included in the server response for exchange metadata. The assertion thatmetadataisNoneis correct and aligns with the expected behavior, mirroring the structure of the corresponding test in theTestBareMetalLoadBalancedCookieAssistantclass.CHANGELOG.md (2)
- 5-9: The changelog entry for version 1.3.1 clearly outlines the changes made to the cookie assistant logic to accommodate the different cookie policies of the Indexer exchange server and the chain server. This entry is concise and informative, providing a clear summary of the changes and their impact.
- 9-9: The changelog entry for version 1.3.0 mentions the removal of
asynciofrom the dependencies. This entry is clear and concise, accurately reflecting a significant change that could affect users and developers working with the software.pyinjective/core/network.py (4)
- 26-29: The modification in the
exchange_metadatamethod to returnNonewhen the cookie is eitherNoneor an empty string is a logical and clean way to handle scenarios where cookies are not applicable. This change ensures that the method's behavior is consistent with the expected functionality in environments where cookies are not used.- 73-73: Setting
cookie_infoto an empty string when no cookie is present in the_fetch_exchange_cookiemethod of theKubernetesLoadBalancedCookieAssistantclass is a sensible default. This change prevents potentialNoneTypeerrors in subsequent operations involvingcookie_info.- 89-94: The addition of a check for the "GCLB" key in the cookie before accessing its expiration time in the
_is_cookie_expiredmethod enhances the robustness of the cookie expiration logic. This change ensures that the method only operates on valid cookie data, preventing potentialKeyErrorexceptions.- 141-141: Similar to the KubernetesLoadBalancedCookieAssistant class, setting
cookie_infoto an empty string when no cookie is present in the_fetch_exchange_cookiemethod of theBareMetalLoadBalancedCookieAssistantclass is a prudent choice. This ensures consistency in handling cookie information across different assistant classes.
…s assistant to tackle the suggestion in the PR review
nicolasbaum
approved these changes
Feb 29, 2024
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- pyinjective/core/network.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- pyinjective/core/network.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests