[CDF-22379] Hosted Extractors: Source #1893
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1893 +/- ##
==========================================
- Coverage 90.78% 90.75% -0.04%
==========================================
Files 126 130 +4
Lines 19700 20104 +404
==========================================
+ Hits 17885 18245 +360
- Misses 1815 1859 +44
|
doctrino
force-pushed
the
hosted-extractors
branch
from
c9c70bd to
1cec0fa
Compare
| def __init__(self, config: ClientConfig, api_version: str | None, cognite_client: CogniteClient) -> None: | ||
| super().__init__(config, api_version, cognite_client) | ||
| self._warning = FeaturePreviewWarning( | ||
| api_maturity="alpha", sdk_maturity="alpha", feature_name="Hosted Extractors" |
There was a problem hiding this comment.
Will add rest and kafka which are alpha, the eventhub and mqtt are beta.
There was a problem hiding this comment.
In this PR? Or a later one?
There was a problem hiding this comment.
Later on, this is 1 /5 while I added kafka and rest in 5/5
| @@ -106,6 +106,7 @@ class APIClient: | |||
| "transformations/(filter|byids|jobs/byids|schedules/byids|query/run)", | |||
| "extpipes/(list|byids|runs/list)", | |||
| "workflows/.*", | |||
| "hostedextractors/.*", | |||
There was a problem hiding this comment.
This now also retries delete and update of sources, is that intended?
| self._warning = FeaturePreviewWarning( | ||
| api_maturity="alpha", sdk_maturity="alpha", feature_name="Hosted Extractors" | ||
| ) |
There was a problem hiding this comment.
Any reason to not move this to HostedExtractorsAPI.__init__?
| self._warning = FeaturePreviewWarning( | |
| api_maturity="alpha", sdk_maturity="alpha", feature_name="Hosted Extractors" | |
| ) | |
| self._warning = FeaturePreviewWarning( | |
| api_maturity="alpha", sdk_maturity="alpha", feature_name="Hosted Extractors" | |
| ) |
There was a problem hiding this comment.
Just convenience. Does not matter much as it will be removed in the future.
| "hostedExtractorsAcl": { | ||
| "actions": ["READ", "WRITE"], | ||
| "scope": {"all": {}}, |
There was a problem hiding this comment.
Add this new ACL? Then we can use the better syntax:
HostedExtractorsAcl(
[HostedExtractorsAcl.Action.Read, HostedExtractorsAcl.Action.Write],
HostedExtractorsAcl.Scope.All,
)There was a problem hiding this comment.
Did not spend the time to update the script to the new syntax.
cognite/client/_api_client.py
Outdated
| @@ -1218,6 +1224,7 @@ def _convert_resource_to_patch_object( | |||
| resource: CogniteResource, | |||
| update_attributes: list[PropertySpec], | |||
| mode: Literal["replace_ignore_null", "patch", "replace"] = "replace_ignore_null", | |||
| identifying_properties: dict[str, Any] | None = None, | |||
There was a problem hiding this comment.
Where does the word "identifying" come from? Is there a more general term we could use in case we need to add "extra-but-not-identifying-properties" in the future?
There was a problem hiding this comment.
Tried another solution. Wdyt?
| @classmethod | ||
| @abstractmethod | ||
| def _load_source(cls, resource: dict[str, Any]) -> Self: | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
I vote to remove this and just use _load for subclasses - but maybe there's a strong reason to keep it this way?
There was a problem hiding this comment.
Yes infinite recursion.
| try: | ||
| return cast(Self, _SOURCE_WRITE_CLASS_BY_TYPE[type_]._load_source(resource)) | ||
| except KeyError: | ||
| raise TypeError(f"Unknown source type: {type_}") |
There was a problem hiding this comment.
I like this pattern - write version raises on "unknown", while read version is lenient! Nice!👌
| def as_write(self, key_value: str | None = None) -> EventHubSourceWrite: | ||
| if key_value is None: | ||
| raise ValueError("key_value must be provided") |
There was a problem hiding this comment.
Perhaps?
| def as_write(self, key_value: str | None = None) -> EventHubSourceWrite: | |
| if key_value is None: | |
| raise ValueError("key_value must be provided") | |
| def as_write(self, key_value: str) -> EventHubSourceWrite: |
There was a problem hiding this comment.
It breaks the overload. Almost tempted to just write a TypeError here instead. You cannot make a write version as there is no key.
tests/tests_integration/test_api/test_hosted_extractors/test_sources.py
Outdated
Show resolved
Hide resolved
| if retrieved: | ||
| return retrieved |
There was a problem hiding this comment.
This will probably never trigger since you use random_string(10). What about
f"{platform.system()}-{platform.python_version()"There was a problem hiding this comment.
Maybe something I am missing here, but why would I need that if I have a random string?
There was a problem hiding this comment.
Point is you will never retrieve it when using a random string, hence make each test runner have its own
There was a problem hiding this comment.
Thanks finally got it now.
| key_value="myKey", | ||
| event_hub_name="myEventHub", | ||
| ) | ||
| retrieved = cognite_client.hosted_extractors.sources.retrieve(my_hub.external_id, ignore_unknown_ids=True) |
There was a problem hiding this comment.
Dont need this when just fetching one, right?
| retrieved = cognite_client.hosted_extractors.sources.retrieve(my_hub.external_id, ignore_unknown_ids=True) | |
| retrieved = cognite_client.hosted_extractors.sources.retrieve(my_hub.external_id) |
There was a problem hiding this comment.
For these endpoints you do
There was a problem hiding this comment.
Why? That breaks with other APIs
tests/tests_integration/test_api/test_hosted_extractors/test_sources.py
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGELOG.md # cognite/client/_version.py # pyproject.toml
| output = super()._convert_resource_to_patch_object(resource, update_attributes, mode) | ||
| if hasattr(resource, "type"): | ||
| output["type"] = resource.type | ||
| return output |
Description
Part 1 of 5 planned. (Mapping, Destination, Job + adding Rest + kafka to Source)
Avoid adding more to keep PR smaller
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.