-
Notifications
You must be signed in to change notification settings - Fork 36
feat(config): honor COGNITE_DISABLE_PYPI_VERSION_CHECK for PyPI version warning #2531
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
Open
andersfylling
wants to merge
1
commit into
master
Choose a base branch
from
andersfylling/cognite-sdk/disable-pypi-version-check-env
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import getpass | ||
| import os | ||
| import pprint | ||
| import re | ||
| import warnings | ||
|
|
@@ -19,7 +20,8 @@ class GlobalConfig: | |
| by the CogniteClient constructor if no config is passed directly. Defaults to None. | ||
| disable_gzip (bool): Whether or not to disable gzipping of json bodies. Defaults to False. | ||
| disable_pypi_version_check (bool): Whether or not to check for newer SDK versions when instantiating a new client. | ||
| Defaults to False. | ||
| Defaults to False. You can also set the environment variable ``COGNITE_DISABLE_PYPI_VERSION_CHECK`` to a truthy | ||
| value (``1``, ``true``, ``yes``, ``on``, case-insensitive) to disable the check without changing this attribute. | ||
| status_forcelist (Set[int]): HTTP status codes to retry. Defaults to {429, 502, 503, 504} | ||
| max_retries (int): Max number of retries on a given http request. Defaults to 10. | ||
| max_retries_connect (int): Max number of retries on connection errors. Defaults to 3. | ||
|
|
@@ -100,6 +102,17 @@ def apply_settings(self, settings: dict[str, Any] | str) -> None: | |
| global_config = GlobalConfig() | ||
|
|
||
|
|
||
| def _env_truthy(name: str) -> bool: | ||
| val = os.environ.get(name) | ||
| if val is None: | ||
| return False | ||
| return val.strip().lower() in {"1", "true", "yes", "on"} | ||
|
|
||
|
|
||
| def _pypi_version_check_disabled() -> bool: | ||
| return global_config.disable_pypi_version_check or _env_truthy("COGNITE_DISABLE_PYPI_VERSION_CHECK") | ||
|
Comment on lines
+112
to
+113
Contributor
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. This function is missing a docstring, which is required by the repository style guide (lines 94, 117). def _pypi_version_check_disabled() -> bool:
"""Check if the PyPI version check is disabled.
The check is disabled if either the global config flag is set or the
corresponding environment variable is set to a truthy value.
Returns:
bool: True if the version check is disabled, False otherwise.
"""
return global_config.disable_pypi_version_check or _env_truthy("COGNITE_DISABLE_PYPI_VERSION_CHECK")References
|
||
|
|
||
|
|
||
| class ClientConfig: | ||
| """Configuration object for the client | ||
|
|
||
|
|
@@ -145,7 +158,7 @@ def __init__( | |
| self.debug = True | ||
| self._validate_config() | ||
|
|
||
| if not global_config.disable_pypi_version_check: | ||
| if not _pypi_version_check_disabled(): | ||
| from cognite.client.utils._version_checker import check_client_is_running_latest_version | ||
|
|
||
| check_client_is_running_latest_version() | ||
|
|
||
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
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
Oops, something went wrong.
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.
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 function is missing a docstring, which is required by the repository style guide (lines 94, 117). Additionally, the set of truthy strings is a magic value and should be defined as a constant with an
UPPER_SNAKE_CASEname, as per the style guide (line 87).Since a module-level constant cannot be added within the scope of this change, defining it inside the function is an acceptable alternative.
References
ArgsandReturnssections for clarity. (link)UPPER_SNAKE_CASE. (link)