feat(integrations): Add startup/checkin endpoints + tooling fixes - #2822
feat(integrations): Add startup/checkin endpoints + tooling fixes#2822vikramlc-cognite wants to merge 1 commit into
Conversation
![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 implements the extractor startup and check-in protocols for the Integrations API, introducing new data classes (StartupRequest, CheckinRequest, CheckinResponse, TaskUpdate, and ErrorWithTask) along with corresponding sync and async client methods. Feedback on the changes suggests removing the custom dump methods in StartupRequest and CheckinRequest because they are redundant and violate the Liskov Substitution Principle by altering the default camel_case parameter value.
| def dump(self, camel_case: bool = True) -> dict[str, Any]: | ||
| result = super().dump(camel_case) | ||
| result["extractor"] = self.extractor.dump(camel_case) | ||
| if self.tasks is not None: | ||
| result["tasks"] = [task.dump(camel_case) for task in self.tasks] | ||
| return result |
There was a problem hiding this comment.
The custom dump method in StartupRequest is redundant and violates the Liskov Substitution Principle (LSP) by changing the default value of camel_case from False to True compared to the base class CogniteResource.dump. Since CogniteResource.dump already automatically and recursively serializes nested CogniteResource attributes (like extractor and tasks), this custom implementation can be safely removed to improve maintainability and consistency.
References
- Maintainability: Write code that is easy to modify and extend. Consistency: Follow established patterns across the codebase. (link)
| def dump(self, camel_case: bool = True) -> dict[str, Any]: | ||
| result = super().dump(camel_case) | ||
| if self.task_events is not None: | ||
| key = "taskEvents" if camel_case else "task_events" | ||
| result[key] = [event.dump(camel_case) for event in self.task_events] | ||
| if self.errors is not None: | ||
| result["errors"] = [error.dump(camel_case) for error in self.errors] | ||
| return result |
There was a problem hiding this comment.
The custom dump method in CheckinRequest is redundant and violates the Liskov Substitution Principle (LSP) by changing the default value of camel_case from False to True compared to the base class CogniteResource.dump. Since CogniteResource.dump already automatically and recursively serializes nested CogniteResource attributes (like task_events and errors), this custom implementation can be safely removed to improve maintainability and consistency.
References
- Maintainability: Write code that is easy to modify and extend. Consistency: Follow established patterns across the codebase. (link)
Adds the extractor self-registration/heartbeat protocol (startup, checkin) now that service-contracts PR #3378 removed their ifdef: internal marking, plus the retry-idempotency registration and codespell config needed to support the "checkin" identifier. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
43aaa1d to
1a77b03
Compare
2 participants
Summary
Adds the extractor self-registration/heartbeat protocol (
startup,checkin) now thatcognitedata/service-contractsPR #3378 removed theirifdef: internalmarking, plus the retry-idempotency registration and codespell config needed to support the "checkin" identifier. Builds on the API client from #2820/#2821.Type of change
What changed
cognite/client/data_classes/integrations/checkin.py:TaskUpdate,ErrorWithTask,StartupRequest,CheckinRequest,CheckinResponse.IntegrationsAPI.startup()andIntegrationsAPI.checkin()methods,POST-ing to/integrations/startupand/integrations/checkinrespectively with the same beta header as the rest of the module.integrations/checkinandintegrations/startupto the non-idempotent POST pattern list inutils/_url.py, with matching cases intest_api_client.py..pre-commit-config.yaml: added--ignore-words-list=checkinto thecodespellhook args, since "checkin" is the API's realoperationId/URL segment and can't be reworded to "check-in" without diverging from the actual wire contract. Passed as a hook arg rather thanpyproject.toml's[tool.codespell]because reading TOML config requirestomli/tomllib, which isn't guaranteed available in the hook's isolated environment on Python <3.11.test_startup/test_checkinintest_api/test_integrations/test_integrations.py, and round-trip tests for all 5 new data classes intest_data_classes/test_integrations.py.Why it changed
cognitedata/service-contractsPR #3378 (removed theifdef: internaltag from these two endpoints) — https://github.com/cognitedata/service-contracts/pull/3378#discussion_r3932420851What to focus on during review
startup()/checkin()are explicitly documented (Note:in the docstring) as normally only called by extractor implementations as part of the integrations protocol, not typical SDK consumers — worth confirming that framing reads clearly to a reviewer unfamiliar with the extractor side.pyproject.toml's[tool.codespell]worked locally but silently failed in CI (Python 3.10 in the hook's isolated venv couldn't parse the TOML withouttomli) — this PR's.pre-commit-config.yamlargs-based fix is the one that's CI-verified.Test evidence
pytest tests/tests_unit/ -q→ 6,708 passed, 8 failed (pre-existing, unrelated: missinggeopandas/sympyin the local dev environment — confirmed viagit stash/pop A-B testing that these fail identically onmaster), 6 skippedpython scripts/sync_client_codegen/main.py verify→ sync mirrors up to dateruff check/ruff format --check→ clean (550 files)mypy→ no issues (550 source files)git diffagainst it is empty)Risks and unknowns
startup/checkinare part of an extractor-facing heartbeat protocol rather than typical end-user SDK surface — low usage risk for most SDK consumers, but worth a sanity check from someone on the extractors side that the request/response shapes match what real extractors send.Rollout and rollback
FeaturePreviewWarninggating as the rest of the module. No migrations. Revert is a straight revert of this commit.Checklist