Lazy load pull secret#32
Conversation
Previously we were querying OCM for the pull secret on every tool call but it's only needed when we create clusters or infraenvs. This commit changes the service client so that it only queries for the pull secret when it is required
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
service_client/assisted_service_api.py (1)
43-48: Consider thread safety for the lazy loading implementation.The lazy loading property implementation is functionally correct but not thread-safe. If multiple threads access
pull_secretsimultaneously, they might both see_pull_secretasNoneand both call_get_pull_secret(), potentially causing duplicate requests or race conditions.For single-threaded usage, this implementation works well. If thread safety is required, consider using a lock or
threading.Lock()to synchronize access.If thread safety is needed, here's a thread-safe implementation:
+import threading + class InventoryClient: def __init__(self, access_token: str): self.access_token = access_token self._pull_secret: Optional[str] = None + self._pull_secret_lock = threading.Lock() # ... rest of init @property def pull_secret(self) -> str: - if self._pull_secret is None: - self._pull_secret = self._get_pull_secret() + if self._pull_secret is None: + with self._pull_secret_lock: + if self._pull_secret is None: # Double-check locking + self._pull_secret = self._get_pull_secret() return self._pull_secret
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service_client/assisted_service_api.py(1 hunks)tests/test_assisted_service_api.py(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-service-mcp-saas-main-on-pull-request
🔇 Additional comments (9)
service_client/assisted_service_api.py (3)
37-37: LGTM: Proper initialization for lazy loading.The private attribute initialization correctly supports the lazy loading pattern by deferring pull secret retrieval until first access.
324-324: LGTM: Correct usage of lazy-loaded pull secret.The pull secret is now accessed via the property, which will trigger lazy loading only when needed during cluster creation.
366-366: LGTM: Correct usage of lazy-loaded pull secret.The pull secret is now accessed via the property, which will trigger lazy loading only when needed during infrastructure environment creation.
tests/test_assisted_service_api.py (6)
82-84: LGTM: Proper test adaptation for lazy loading.The test correctly triggers lazy loading by accessing the
pull_secretproperty and storing the result before assertion. This ensures the lazy loading behavior is properly tested.Also applies to: 90-90
99-99: LGTM: Correct exception testing for lazy loading.The test properly creates the client first and then accesses the
pull_secretproperty to trigger the exception during lazy loading, which accurately reflects the new behavior.Also applies to: 103-103
116-116: LGTM: Consistent lazy loading test pattern.The test follows the same correct pattern of creating the client first, then accessing the property to trigger lazy loading with the custom URL.
Also applies to: 119-119
368-371: LGTM: Comprehensive mocking for lazy loading.The test correctly patches both
_installer_apiand_get_pull_secretmethods to ensure consistent mocking during cluster creation. The assertion properly expects the mocked pull secret value.Also applies to: 387-387
401-404: LGTM: Consistent mocking pattern for single node cluster test.The test applies the same correct mocking pattern for both API methods, ensuring consistent behavior for single node cluster creation tests.
428-431: LGTM: Proper mocking for infrastructure environment creation.The test correctly patches both required methods to ensure consistent mocking during infrastructure environment creation, maintaining the same pattern as the cluster creation tests.
1 participant
Previously we were querying OCM for the pull secret on every tool call but it's only needed when we create clusters or infraenvs. This commit changes the service client so that it only queries for the pull secret when it is required
Summary by CodeRabbit
Refactor
Tests