Conversation
Contributor
📝 WalkthroughWalkthroughset_integration_env() was updated to call clear_config_cache() after applying environment variables fetched from integrations so that subsequent get_config() reads updated values. Tests were added: one verifies that clearing the config cache lets a late-set env var affect get_config(), and another asserts set_integration_env() calls clear_config_cache() once when integrations provide variables. Sequence Diagram(s)sequenceDiagram
participant Script as set_integration_env()
participant HTTP as Integration API
participant Env as OS Environment
participant Config as config.get_config / cache
Script->>HTTP: fetch integration vars
HTTP-->>Script: returns vars (e.g. DEEPNOTE_DO_NOT_COERCE_FLOAT)
Script->>Env: set environment variables
Script->>Config: clear_config_cache()
Note right of Config: next get_config() will re-read env
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
=======================================
Coverage 74.32% 74.32%
=======================================
Files 94 94
Lines 5534 5535 +1
Branches 824 824
=======================================
+ Hits 4113 4114 +1
Misses 1155 1155
Partials 266 266
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_config.py`:
- Line 30: Add an explicit return type annotation to the test function signature
for test_coerce_float_picks_up_late_env_after_cache_clear by changing its def
line to include "-> None"; update the function definition of
test_coerce_float_picks_up_late_env_after_cache_clear to declare a None return
type to comply with the project's type-hinting guideline.
In `@tests/unit/test_set_integrations_env.py`:
- Around line 102-107: Replace the loose invocation check in the test so it
asserts exactly one call: in the block that patches
"deepnote_toolkit.set_integrations_env.clear_config_cache" and invokes
set_integration_env(), change the assertion from using mock_clear.called to
calling mock_clear.assert_called_once() to ensure the cache-clear function was
invoked exactly once.
- Around line 81-95: Update the new test function and its nested dummy classes
to include explicit type hints: annotate the test function parameters
(mock_get_config, monkeypatch) with their expected types, add return type
annotations for DummyResp.json (-> list[dict[str, str]] or appropriate type) and
for DummySession.get (-> DummyResp) and DummySession.mount (-> None), and
annotate any method arguments (e.g., *a: Any, **k: Any) so all functions and
methods in test_set_integration_env_clears_config_cache, DummyResp.json,
DummySession.mount, and DummySession.get have explicit parameter and return type
hints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d96bd3e9-4e88-47bb-8506-d581016bd0eb
📒 Files selected for processing (3)
deepnote_toolkit/set_integrations_env.pytests/unit/test_config.pytests/unit/test_set_integrations_env.py
|
🚀 Review App Deployment Started
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/test_set_integrations_env.py (1)
81-95:⚠️ Potential issue | 🟡 MinorAdd explicit type hints in the new test segment.
Line 81, Line 87, Line 91, and Line 94 are still missing annotations in the new test and nested dummy methods.
Proposed fix
+from typing import Any import types from unittest import mock @@ def test_set_integration_env_clears_config_cache( - mock_get_config, monkeypatch # noqa: ARG001 -): + mock_get_config: mock.MagicMock, monkeypatch: pytest.MonkeyPatch # noqa: ARG001 +) -> None: class DummyResp: ok = True - def json(self): + def json(self) -> list[dict[str, str]]: return [{"name": "DEEPNOTE_DO_NOT_COERCE_FLOAT", "value": "1"}] class DummySession: - def mount(self, *a, **k): + def mount(self, *a: Any, **k: Any) -> None: pass - def get(self, *a, **k): + def get(self, *a: Any, **k: Any) -> DummyResp: return DummyResp()#!/bin/bash ruff check tests/unit/test_set_integrations_env.py --select ANN,ARGAs per coding guidelines, "Use explicit type hints for function parameters and return values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_set_integrations_env.py` around lines 81 - 95, Add explicit type annotations to the new test and its nested dummy classes: annotate the test function parameters in test_set_integration_env_clears_config_cache (mock_get_config, monkeypatch) with appropriate types (e.g., fixtures' types or typing.Any), add a return type for DummyResp.json (-> list[dict] or -> Any) and for DummySession.mount and DummySession.get (-> None and -> DummyResp or -> Any respectively). Ensure imports from typing are added if needed and use the exact symbol names DummyResp.json, DummySession.mount, DummySession.get, and test_set_integration_env_clears_config_cache to locate where to add the annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_set_integrations_env.py`:
- Around line 100-107: This test can leak the DEEPNOTE_DO_NOT_COERCE_FLOAT
environment variable set by set_integration_env(); before calling
set_integration_env() in the test (which currently uses monkeypatch and mocks
clear_config_cache), capture the original value of DEEPNOTE_DO_NOT_COERCE_FLOAT
(e.g., via os.environ.get or monkeypatch) and after the assertion restore it to
its prior state (use monkeypatch.setenv or monkeypatch.delenv as appropriate) so
the environment is unchanged for subsequent tests; keep references to
set_integration_env and the existing mock_clear.assert_called_once() intact.
---
Duplicate comments:
In `@tests/unit/test_set_integrations_env.py`:
- Around line 81-95: Add explicit type annotations to the new test and its
nested dummy classes: annotate the test function parameters in
test_set_integration_env_clears_config_cache (mock_get_config, monkeypatch) with
appropriate types (e.g., fixtures' types or typing.Any), add a return type for
DummyResp.json (-> list[dict] or -> Any) and for DummySession.mount and
DummySession.get (-> None and -> DummyResp or -> Any respectively). Ensure
imports from typing are added if needed and use the exact symbol names
DummyResp.json, DummySession.mount, DummySession.get, and
test_set_integration_env_clears_config_cache to locate where to add the
annotations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a30774d-53e6-4cb6-8308-8d637b974845
📒 Files selected for processing (1)
tests/unit/test_set_integrations_env.py
m1so
reviewed
Apr 9, 2026
…r after cache clear
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/test_config.py (1)
30-30: 🛠️ Refactor suggestion | 🟠 MajorAdd explicit type hints to the test signature.
Please type the fixture and return explicitly, e.g.
monkeypatch: pytest.MonkeyPatchand-> None.Proposed fix
-def test_coerce_float_picks_up_late_env_after_cache_clear(monkeypatch): +def test_coerce_float_picks_up_late_env_after_cache_clear( + monkeypatch: pytest.MonkeyPatch, +) -> None:As per coding guidelines, "Use explicit type hints for function parameters and return values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_config.py` at line 30, The test function test_coerce_float_picks_up_late_env_after_cache_clear is missing explicit type hints on its signature; update the parameter and return types to follow guidelines by annotating monkeypatch as pytest.MonkeyPatch and the function return as -> None (e.g., monkeypatch: pytest.MonkeyPatch and -> None) so the test signature is explicitly typed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/test_config.py`:
- Line 30: The test function
test_coerce_float_picks_up_late_env_after_cache_clear is missing explicit type
hints on its signature; update the parameter and return types to follow
guidelines by annotating monkeypatch as pytest.MonkeyPatch and the function
return as -> None (e.g., monkeypatch: pytest.MonkeyPatch and -> None) so the
test signature is explicitly typed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7c6708a-7e3f-4e40-969a-927995e861c8
📒 Files selected for processing (1)
tests/unit/test_config.py
m1so
approved these changes
Apr 10, 2026
…egration-env-var-injection
mfranczel
deleted the
michal/blu-5912-clear-config-cache-after-integration-env-var-injection
branch
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
3 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.
Problem
get_config()is memoized via@lru_cache. During runtime init, it gets called (and cached) beforeset_integration_env()fetches and injects integration env vars likeDEEPNOTE_DO_NOT_COERCE_FLOAT. The cached config never sees the late-arriving variable.Fix
Call
clear_config_cache()at the end ofset_integration_env()after env vars are written, so the nextget_config()re-reads the environmentSummary by CodeRabbit
Bug Fixes
Tests