-
Notifications
You must be signed in to change notification settings - Fork 38
Remove support for schemas that are loaded from a URL (breaking) #412
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
base: main
Are you sure you want to change the base?
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 |
|---|---|---|
|
|
@@ -19,7 +19,6 @@ | |
| from typing import Any, NamedTuple, Optional, Union | ||
|
|
||
| import json5 | ||
| import requests | ||
|
|
||
| from .credential_manager import CredentialManager | ||
| from .custom_warnings import ProfileNotFoundWarning, ProfileParsingWarning | ||
|
|
@@ -180,15 +179,11 @@ def schema_list(self, cwd: Optional[str] = None) -> list[dict[str, Any]]: | |
| schema_json: dict[str, Any] = {} | ||
|
|
||
| if schema.startswith(("https://", "http://")): | ||
| try: | ||
| response = requests.get(schema) | ||
| response.raise_for_status() # Ensure it's a valid response | ||
| schema_json = response.json() | ||
| except requests.RequestException as e: | ||
| if not self.__suppress_config_file_warnings: | ||
| warnings.warn(f"Invalid schema request: {e}") | ||
| self.__logger.warning(f"Invalid schema request: {e}") | ||
| return [] | ||
| # remote schema loading is not supported | ||
| if not self.__suppress_config_file_warnings: | ||
| warnings.warn(f"Loading a JSON schema from a remote URL is not supported: {schema}") | ||
| self.__logger.warning(f"Loading a JSON schema from a remote URL is not supported: {schema}") | ||
| return [] | ||
|
Member
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. I think we should be raising the error regardless. Right now, the only place that's calling the If we return an empty list here, the developer/user may not realize that their environment variables were not loaded because the schema comes from a URL. This leads to unexpected behavior. |
||
|
|
||
| elif schema.startswith("file://") or os.path.isfile(schema): | ||
| try: | ||
|
|
||
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.
We seem to be using this same string in multiple paces... wondering if we could move it somewhere else and reuse it.