refactor: standardize by renaming PLUTO_API_TOKEN to PLUTO_API_KEY#58
refactor: standardize by renaming PLUTO_API_TOKEN to PLUTO_API_KEY#58
Conversation
Summary of ChangesHello @Nicoowr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request standardizes the environment variable used for API authentication within the Pluto ecosystem. By renaming Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes the API authentication environment variable by renaming PLUTO_API_TOKEN to PLUTO_API_KEY. The changes are consistent across the codebase, including documentation, application code, and tests. However, the current implementation is a breaking change as it completely removes support for the old PLUTO_API_TOKEN variable. My main feedback is to introduce backward compatibility by continuing to support PLUTO_API_TOKEN with a deprecation warning, similar to how older MLOP_* variables are handled. This will ensure a smoother transition for existing users.
| def _resolve_api_token() -> Optional[str]: | ||
| """Resolve API token from environment or keyring.""" | ||
| token = os.environ.get('PLUTO_API_TOKEN') | ||
| token = os.environ.get('PLUTO_API_KEY') |
There was a problem hiding this comment.
This change removes support for PLUTO_API_TOKEN entirely, which is a breaking change. To provide a smoother transition for users, consider maintaining backward compatibility by supporting PLUTO_API_TOKEN with a deprecation warning, similar to how MLOP_API_TOKEN is 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.
@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.
Thanks for the clarification, Nicoowr. I understand that PLUTO_API_KEY might have been present or used in other parts of the codebase, such as pluto/compat/neptune.py. However, the concern here is specifically about the _resolve_api_token function in pluto/query.py.
Before this change, _resolve_api_token explicitly looked for PLUTO_API_TOKEN as its primary environment variable. By changing line 640 from os.environ.get('PLUTO_API_TOKEN') to os.environ.get('PLUTO_API_KEY') and not adding PLUTO_API_TOKEN as a fallback, any user who has PLUTO_API_TOKEN set but not PLUTO_API_KEY will 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_token function to check for PLUTO_API_TOKEN after PLUTO_API_KEY, but with a deprecation warning. This aligns with the approach already taken for MLOP_API_TOKEN.
The revised resolution order for _resolve_api_token should be:
- Check for
PLUTO_API_KEY. - If not found, check for
PLUTO_API_TOKENand issue a deprecation warning. - If not found, check for
MLOP_API_TOKENand issue a deprecation warning.
Here's a conceptual example of how the _resolve_api_token function could be structured to achieve this:
def _resolve_api_token() -> Optional[str]:
"""Resolve API token from environment or keyring."""
token = os.environ.get('PLUTO_API_KEY')
if token:
return token
token = os.environ.get('PLUTO_API_TOKEN')
if token:
warnings.warn(
'PLUTO_API_TOKEN is deprecated. Use PLUTO_API_KEY instead.',
DeprecationWarning,
stacklevel=3,
)
return token
token = os.environ.get('MLOP_API_TOKEN')
if token:
warnings.warn(
'MLOP_API_TOKEN is deprecated. Use PLUTO_API_KEY instead.',
DeprecationWarning,
stacklevel=3,
)
return token
# ... (keyring lookup logic)
return None| # 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.
Similar to my comment in pluto/query.py, this logic removes support for PLUTO_API_TOKEN, which is a breaking change. For backward compatibility, please consider supporting PLUTO_API_TOKEN with a deprecation warning.
The recommended resolution order is PLUTO_API_KEY, then PLUTO_API_TOKEN, then MLOP_API_TOKEN.
| 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' |
There was a problem hiding this comment.
If you implement my suggestion to add backward compatibility for PLUTO_API_TOKEN, please update the tests to cover the new logic. Specifically, this test should be expanded to ensure PLUTO_API_KEY has precedence over both PLUTO_API_TOKEN and MLOP_API_TOKEN. Additionally, new tests should be added to verify:
- A deprecation warning is issued when
PLUTO_API_TOKENis used. PLUTO_API_TOKENhas precedence overMLOP_API_TOKEN.
|
thank you @Nicoowr ! merging |
2 participants
No description provided.