chore(jobs): remove data_refresh_table and data_refresh_connection job types - #179
chore(jobs): remove data_refresh_table and data_refresh_connection job types#179hotdata-automation[bot] wants to merge 1 commit into
Conversation
|
|
||
| ### Changed | ||
|
|
||
| - chore(jobs): remove data_refresh_table and data_refresh_connection job types |
There was a problem hiding this comment.
Blocking: the changelog entry describes only the job-type removal, but this PR also deletes the entire SecretsApi, ConnectionTypesApi, and RefreshApi, removes ConnectionsApi.check_connection_health, and drops ~20 public models (CreateSecretRequest, SecretMetadataResponse, RefreshRequest, ConnectionTypeSummary, ConnectionHealthResponse, …). Those are all exported from hotdata/__init__.py today, so this is a hard breaking change for any caller importing them.
Either the removals are unintended (in which case the spec sync pulled in more than the linked upstream PR describes), or they need to be documented. If intended, please:
- move them under a
### Removedheading with a Breaking: marker, matching the style of the0.10.0JWT-exchange entry - update the PR title/description, which currently reads as a narrow job-type cleanup
Also note the version is still 0.10.0; a breaking removal set should carry the corresponding bump before release.
There was a problem hiding this comment.
Review
Note on context: the full diff was unavailable in the review prompt and
gh pr diffrefuses this PR (HTTP 406: diff exceeded the maximum number of files (300)); the changed-file list was also truncated (the API returns only the first 100 of 326 paths). I reviewed by reading the post-merge checkout directly —hotdata/models/job_type.py,hotdata/models/job_result.py,hotdata/api/*.py,CHANGELOG.md,README.md,.github/workflows/integration-tests.yml, and the wholetests/integration/suite — and by grepping the tree for references to every removed symbol. The job-type/job-result changes named in the title are internally consistent; the findings below are about everything else this PR removes.
Blocking Issues
1. tests/integration/conftest.py imports three deleted modules — the entire integration suite fails at collection.
tests/integration/conftest.py:17: from hotdata.api.connection_types_api import ConnectionTypesApi
tests/integration/conftest.py:25: from hotdata.api.refresh_api import RefreshApi
tests/integration/conftest.py:27: from hotdata.api.secrets_api import SecretsApi
All three modules are deleted by this PR and no longer exist in hotdata/api/. .github/workflows/integration-tests.yml:99 runs pytest tests/integration -v, so this is an ImportError during conftest import — every integration test errors out, not just the ones that touch these APIs. The secrets_api / connection_types_api / refresh_api fixtures at lines 155, 170, and 190 need to go with them.
2. tests/integration/test_secrets_crud.py and tests/integration/test_connection_types_read.py target removed APIs.
Both import deleted modules at file scope (test_secrets_crud.py:15,17,18, test_connection_types_read.py:9) and exercise endpoints the SDK no longer exposes. They should be deleted alongside the generated test/test_secret_*.py / test/test_connection_type_*.py files this PR already removes.
Worth checking against the upstream scenario manifest: scenario-parity passed, but that check only flags scenarios missing a test file (integration-tests.yml:49-51) — it cannot catch a test file for a scenario that no longer exists. If secrets_crud and connection_types_read are still in api/test-scenarios.yaml, removing these files will flip that check red, which means the manifest needs the matching update upstream first.
3. tests/integration/test_connections_read.py:31 calls check_connection_health, which this PR removes from ConnectionsApi.
health = connections_api.check_connection_health(connection_id)ConnectionsApi no longer defines it (the -267 lines in hotdata/api/connections_api.py, plus the deletion of ConnectionHealthResponse). A grep for check_connection_health across the tree now matches this line and nothing else. Even once finding 1 is fixed, this test fails with AttributeError.
4. Undocumented breaking removals — see the inline comment on CHANGELOG.md:12.
The changelog and PR title cover only the job-type removal, while the PR also deletes SecretsApi, ConnectionTypesApi, RefreshApi, ConnectionsApi.check_connection_health, and ~20 exported models.
Action Required
- Confirm the Secrets / ConnectionTypes / Refresh / connection-health removals are actually intended. The linked upstream PR is titled as a job-type cleanup, and a spec sync that silently drops four API surfaces is worth double-checking before it lands.
- If intended: drop the three dead fixtures and their imports from
tests/integration/conftest.py, deletetest_secrets_crud.pyandtest_connection_types_read.py, remove thecheck_connection_healthassertions fromtest_connections_read.py, and coordinate the scenario-manifest update upstream. - Document the removals in
CHANGELOG.mdunder aRemovedheading with a Breaking: marker, and update the PR title/description to reflect the real scope.
Nits
README.md:3 still advertises secrets in the list of covered API surfaces (… SQL queries, results, secrets, uploads, indexes, jobs …). Drop it if the Secrets API removal is intended. (not blocking)
Auto-generated from the updated HotData OpenAPI spec.
Source: https://github.com/hotdata-dev/www.hotdata.dev/pull/389