-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: standardize by renaming PLUTO_API_TOKEN to PLUTO_API_KEY #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,9 +258,9 @@ def setup(settings: Union[Settings, Dict[str, Any], None] = None) -> Settings: | |
| env_value if env_value is not None else default_value | ||
| ) | ||
|
|
||
| # Read PLUTO_API_TOKEN environment variable (with MLOP_API_TOKEN fallback) | ||
| # Read PLUTO_API_KEY environment variable (with MLOP_API_TOKEN fallback) | ||
| # Only apply if not already set via function parameters | ||
| env_api_token = _get_env_with_deprecation('PLUTO_API_TOKEN', 'MLOP_API_TOKEN') | ||
| env_api_token = _get_env_with_deprecation('PLUTO_API_KEY', 'MLOP_API_TOKEN') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to my comment in The recommended resolution order is |
||
| if env_api_token is not None and '_auth' not in settings_dict: | ||
| new_settings._auth = env_api_token | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
| def _clean_env(monkeypatch): | ||
| """Remove query-related env vars for test isolation.""" | ||
| for key in ( | ||
| 'PLUTO_API_TOKEN', | ||
| 'PLUTO_API_KEY', | ||
| 'MLOP_API_TOKEN', | ||
| 'PLUTO_URL_API', | ||
| 'MLOP_URL_API', | ||
|
|
@@ -42,7 +42,7 @@ def _make(status_code=200, json_data=None, text=''): | |
| @pytest.fixture() | ||
| def client(monkeypatch): | ||
| """A Client with a mocked httpx.Client.""" | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'test-token-123') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'test-token-123') | ||
| c = Client() | ||
| c._client = MagicMock(spec=httpx.Client) | ||
| return c | ||
|
|
@@ -55,7 +55,7 @@ def client(monkeypatch): | |
|
|
||
| class TestResolveApiToken: | ||
| def test_from_pluto_env(self, monkeypatch): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc') | ||
| assert _resolve_api_token() == 'plt_abc' | ||
|
|
||
| def test_from_deprecated_mlop_env(self, monkeypatch): | ||
|
|
@@ -69,7 +69,7 @@ def test_from_deprecated_mlop_env(self, monkeypatch): | |
| assert any('MLOP_API_TOKEN' in str(x.message) for x in w) | ||
|
|
||
| def test_pluto_takes_precedence(self, monkeypatch): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'new_token') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'new_token') | ||
| monkeypatch.setenv('MLOP_API_TOKEN', 'old_token') | ||
| assert _resolve_api_token() == 'new_token' | ||
|
Comment on lines
71
to
74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you implement my suggestion to add backward compatibility for
|
||
|
|
||
|
|
@@ -138,7 +138,7 @@ def test_requires_token(self): | |
| Client() | ||
|
|
||
| def test_from_env(self, monkeypatch): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc') | ||
| c = Client() | ||
| assert c._api_token == 'plt_abc' | ||
| c.close() | ||
|
|
@@ -149,12 +149,12 @@ def test_explicit_token(self, monkeypatch): | |
| c.close() | ||
|
|
||
| def test_context_manager(self, monkeypatch): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc') | ||
| with Client() as c: | ||
| assert c._api_token == 'plt_abc' | ||
|
|
||
| def test_custom_host(self, monkeypatch): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'plt_abc') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'plt_abc') | ||
| c = Client(host='10.0.0.1') | ||
| assert c._url_api == 'http://10.0.0.1:3001' | ||
| c.close() | ||
|
|
@@ -463,7 +463,7 @@ def test_connection_error_retries(self, client): | |
|
|
||
| class TestModuleFunctions: | ||
| def test_list_runs_creates_default_client(self, monkeypatch, mock_response): | ||
| monkeypatch.setenv('PLUTO_API_TOKEN', 'test-token') | ||
| monkeypatch.setenv('PLUTO_API_KEY', 'test-token') | ||
|
|
||
| import pluto.query as pq | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change removes support for
PLUTO_API_TOKENentirely, which is a breaking change. To provide a smoother transition for users, consider maintaining backward compatibility by supportingPLUTO_API_TOKENwith a deprecation warning, similar to howMLOP_API_TOKENis handled.The recommended resolution order would be:
PLUTO_API_KEYPLUTO_API_TOKEN(with deprecation warning)MLOP_API_TOKEN(with deprecation warning)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gemini-code-assist PLUTO_API_KEY was there before (in pluto/compat/neptune.py) so it shouldn't break anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification, Nicoowr. I understand that
PLUTO_API_KEYmight have been present or used in other parts of the codebase, such aspluto/compat/neptune.py. However, the concern here is specifically about the_resolve_api_tokenfunction inpluto/query.py.Before this change,
_resolve_api_tokenexplicitly looked forPLUTO_API_TOKENas its primary environment variable. By changing line 640 fromos.environ.get('PLUTO_API_TOKEN')toos.environ.get('PLUTO_API_KEY')and not addingPLUTO_API_TOKENas a fallback, any user who hasPLUTO_API_TOKENset but notPLUTO_API_KEYwill find that their API token is no longer resolved by this function. This constitutes a breaking change for those users.To maintain backward compatibility and provide a smoother transition, I recommend modifying the
_resolve_api_tokenfunction to check forPLUTO_API_TOKENafterPLUTO_API_KEY, but with a deprecation warning. This aligns with the approach already taken forMLOP_API_TOKEN.The revised resolution order for
_resolve_api_tokenshould be:PLUTO_API_KEY.PLUTO_API_TOKENand issue a deprecation warning.MLOP_API_TOKENand issue a deprecation warning.Here's a conceptual example of how the
_resolve_api_tokenfunction could be structured to achieve this: