[CDF-26674] 😃Extraction pipeline crud#2521
Conversation
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. |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2521 +/- ##
=======================================
Coverage 85.87% 85.87%
=======================================
Files 408 410 +2
Lines 34021 34053 +32
=======================================
+ Hits 29215 29243 +28
- Misses 4806 4810 +4
🚀 New features to boost your workflow:
|
doctrino
changed the title
|
/gemini review |
![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 refactors the extraction pipeline and its configuration CRUD logic to use the new tool client and Pydantic models. This is a positive change that improves code consistency, readability, and maintainability by aligning with the new client architecture. The code is now cleaner and more straightforward.
However, I've identified a critical issue with the delete method for ExtractionPipelineConfigCRUD, which is misleading as it doesn't perform any deletion. I've also noted a significant change in the behavior of the retrieve method for the same class, which now fetches all revisions instead of just the latest one. Please see my detailed comments.
cognite_toolkit/_cdf_tk/cruds/_resource_cruds/extraction_pipeline.py
Outdated
Show resolved
Hide resolved
This reverts commit 2abdd8e.
doctrino
force-pushed
the
extraction-pipeline-crud
branch
from
cf332ef to
2ae88f5
Compare
| def delete(self, items: Sequence[ExtractionPipelineConfigId]) -> list[ExtractionPipelineConfigResponse]: | ||
| """Delete configuration revisions. This is called revert in the CDF API, as it reverts | ||
| the configuration to a previous revision. | ||
|
|
||
| Args: | ||
| items: List of ExtractionPipelineConfigId objects to delete. | ||
|
|
||
| Returns: | ||
| List of the latest configuration revisions after the revert. | ||
|
|
||
| """ | ||
| endpoint = self._method_endpoint_map["delete"] | ||
| latest_new_configurations: list[ExtractionPipelineConfigResponse] = [] | ||
| for item in items: | ||
| request = RequestMessage( | ||
| endpoint_url=self._make_url(endpoint.path), | ||
| method=endpoint.method, | ||
| body_content=item.dump(), | ||
| ) | ||
| response = self._http_client.request_single_retries(request).get_success_or_raise() | ||
| latest_new_configurations.append(ExtractionPipelineConfigResponse.model_validate_json(response.body)) | ||
| return latest_new_configurations |
There was a problem hiding this comment.
To make sure: the API returns the list of revisions remaining after delete?
I'm not sure I follow the connection between delete and revert here. Revisions are immutable, right? So we (the API that is), implicitly takes the newest (one) that was not part of the delete request?
There was a problem hiding this comment.
No, it returns the latest active one. I call it delete to have a standardize interface on all API methods.
2 participants
Description
Please describe the change you have made.
Bump
Changelog
Improved