Fixes #29768: refactor(ingestion): migrate S3 test-connection to declarative framework#29770
Fixes #29768: refactor(ingestion): migrate S3 test-connection to declarative framework#29770IceS2 wants to merge 5 commits into
Conversation
…arative framework Migrate the S3 storage connector's test-connection off the legacy test_connection_steps helper onto the declarative @check / ChecksProvider / ErrorPack framework, and seed the shared Storage-vertical scaffolding.
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Migrates the S3 storage connector’s test-connection implementation from the legacy test_connection_steps flow to the declarative @check / ChecksProvider / ErrorPack framework, and introduces shared storage-vertical check helpers to support future storage connector migrations.
Changes:
- Added storage-vertical scaffolding (
StorageStep+ shared boto3-style helpers) undercore/connections/test_connection/checks/storage.py. - Refactored S3 test-connection to a
S3Checksprovider with anS3_ERRORSErrorPack, removing the legacytest_connectionoverride. - Updated the shipped S3 test-connection definition and added unit tests to assert step coverage + error classification behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openmetadata-service/src/main/resources/json/data/testConnections/storage/s3.json | Marks ListBuckets as a ConnectionGate category to allow gating/short-circuit behavior. |
| ingestion/src/metadata/core/connections/test_connection/checks/storage.py | Introduces shared storage check steps and boto3-style helper functions returning Evidence / raising CheckError. |
| ingestion/src/metadata/ingestion/source/storage/s3/connection.py | Implements S3Checks + S3_ERRORS and wires S3 connection into the declarative checks runner. |
| ingestion/tests/unit/source/storage/s3/test_connection.py | Adds/updates unit tests to validate S3 checks align with the shipped definition and that error classification works as expected. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _SEED = ( | ||
| Path(__file__).parents[6] / "openmetadata-service/src/main/resources/json/data" / "testConnections/storage/s3.json" | ||
| ) |
There was a problem hiding this comment.
parents[6] encodes the assumption that test_connection.py sits exactly seven directory levels deep from the repo root. If the test file or the ingestion/ tree is ever reorganised, the path resolves silently to the wrong directory (no import error — only a FileNotFoundError when the test actually runs). A common alternative is to walk upward until a sentinel file (e.g., .git or pyproject.toml) is found, or to resolve the path relative to a known package root via importlib.resources.
| command = "s3:ListBuckets" | ||
| try: | ||
| response = client.list_buckets(MaxBuckets=limit) # pyright: ignore[reportAttributeAccessIssue] | ||
| except Exception as cause: | ||
| raise CheckError(cause, Evidence(command=command)) from cause | ||
| buckets = response.get("Buckets", []) | ||
| caveat = None | ||
| if not buckets: | ||
| caveat = Diagnosis( | ||
| title="No buckets visible", | ||
| remediation="Verify the identity can list buckets, or configure bucketNames explicitly.", | ||
| ) | ||
| summary = f"{_count(len(buckets), 'bucket')} enumerated" + _more_suffix( | ||
| len(buckets), bool(response.get("ContinuationToken")) | ||
| ) | ||
| return Evidence(summary=summary, command=command, caveat=caveat) |
| Capped at ``limit`` via ``MaxBuckets``; when the account holds more, the | ||
| response carries a ``ContinuationToken`` and the summary says so. |
| assert evidence.summary == "2 buckets enumerated" | ||
| assert evidence.command == "s3:ListBuckets" | ||
| assert evidence.caveat is None | ||
| client.s3_client.list_buckets.assert_called_once_with(MaxBuckets=100) | ||
| client.s3_client.list_objects.assert_not_called() | ||
|
|
||
|
|
||
| def test_check_buckets_flags_truncation_beyond_the_cap(): | ||
| client = MagicMock() | ||
| client.s3_client.list_buckets.return_value = { | ||
| "Buckets": [{"Name": "a"}], | ||
| "ContinuationToken": "more", | ||
| } | ||
|
|
||
| evidence = _checks(client).check_buckets() | ||
|
|
||
| assert "more exist" in evidence.summary | ||
|
|
Code Review ✅ Approved 3 resolved / 3 findingsMigrates S3 test-connection to the declarative framework, establishing reusable storage scaffolding and comprehensive error handling. No issues found. ✅ 3 resolved✅ Edge Case: list_buckets docstring misstates behavior for missing permissions
✅ Quality: InvalidClientTokenId diagnosis has no unit test
✅ Edge Case: list_buckets now sends MaxBuckets, may break S3-compatible stores
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 similar comment
Code Review ✅ Approved 3 resolved / 3 findingsMigrates S3 test-connection to the declarative framework, establishing reusable storage scaffolding and comprehensive error handling. No issues found. ✅ 3 resolved✅ Edge Case: list_buckets docstring misstates behavior for missing permissions
✅ Quality: InvalidClientTokenId diagnosis has no unit test
✅ Edge Case: list_buckets now sends MaxBuckets, may break S3-compatible stores
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
🔴 Playwright Results — 2 failure(s), 21 flaky✅ 4497 passed · ❌ 2 failed · 🟡 21 flaky · ⏭️ 37 skipped
Genuine Failures (failed on all attempts)❌
|
Fixes #29768
Migrates the S3 storage connector's test-connection off the legacy
test_connection_stepshelper onto the declarative@check/ChecksProvider/ErrorPackframework. S3 is the first Storage-vertical connector to migrate, so it also seeds the shared scaffolding the remaining storage connectors (gcs, …) will reuse.What changed
core/connections/test_connection/checks/storage.py(first-occurrence Storage scaffolding, modeled onchecks/database.py): aStorageStepenum (ListBuckets,GetMetrics) plus boto3-generic helperslist_buckets/probe_buckets/list_metricsthat returnEvidenceand raiseCheckError(carrying the attempted API operation as thecommand) on failure.command/summaryare free text (e.g.s3:ListBuckets) since there is no SQL.s3/connection.py:S3Checksprovider with a@checkper step, andS3_ERRORS— a botocoreErrorPackmatching AWSClientErrorcodes (InvalidAccessKeyId,SignatureDoesNotMatch,UnrecognizedClientException,ExpiredToken,NoSuchBucket,AccessDenied) and client-side botocore types (NoCredentialsError,EndpointConnectionError), folding in the sharedNETWORK_ERRORSpack. The oldtest_connectionoverride is removed;checks()drives the runner. The client is built lazily inside the checks (an assume-role config calls STS on session creation) so nothing touches the network before the gate.testConnections/storage/s3.json:ListBucketsgainscategory: ConnectionGateso a failed access step short-circuits.The module-level
get_connection(...) -> S3ObjectStoreClientis preserved (the sampler imports it);_get_clientdelegates to it.Storage-vertical helpers here are intentionally not extracted from any existing shared code — a later dedicated pass can consolidate across storage connectors as more migrate.
Greptile Summary
This PR migrates the S3 storage connector's test-connection logic from the legacy
test_connection_stepshelper to the declarative@check/ChecksProvider/ErrorPackframework, and adds the first shared Storage-category scaffolding (checks/storage.py) that future GCS and other storage connectors will reuse.checks/storage.py: New shared Storage helper withStorageStepenum (ListBuckets,GetMetrics) and three helper functions —list_buckets,probe_buckets,list_metrics— that returnEvidenceand raiseCheckErroron failure, keeping behavior compatible with S3-compatible stores by using parameter-less boto3 calls.s3/connection.py: Replaces the oldtest_connectionoverride withS3Checks(aChecksProvider) carryingS3_ERRORS— a botocoreErrorPackcovering nine distinct AWS error codes/types — and achecks()factory that builds the provider lazily to avoid touching STS before the runner gate.s3.json+ tests:ListBucketsstep gainscategory: ConnectionGatefor proper short-circuit behaviour; the colocated test suite exercises each check path and each error-pack classification.Confidence Score: 5/5
Safe to merge — the migration is a clean behavioural drop-in of the old test_connection override, the lazy-client pattern is correct, and the existing sampler import path (get_connection) is untouched.
The change is well-scoped: it replaces one method on S3Connection, adds a shared storage helper module, and seeds the JSON definition with the missing category flag. The new checks reproduce the old logic faithfully (probe configured buckets or list all; then list CloudWatch metrics), error classification is comprehensive and tested against all nine botocore failure modes, and the lazy-connect lambda correctly defers STS calls until the runner gate opens. No regressions were found in the sampler path or the base connection lifecycle.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UC as User/Automation participant BC as BaseConnection.test_connection() participant S3C as S3Connection.checks() participant S3Ch as S3Checks participant TCR as TestConnectionRunner participant AWS as AWS (S3 / CloudWatch) UC->>BC: test_connection(metadata, workflow) BC->>S3C: checks() S3C-->>BC: "S3Checks(connect=lambda: self.client, bucket_names=...)" BC->>TCR: run(metadata, workflow) TCR->>TCR: fetch definition (s3.json steps) Note over TCR: Step 1: ListBuckets (ConnectionGate) TCR->>S3Ch: check_buckets() S3Ch->>AWS: "list_buckets() or list_objects(Bucket, MaxKeys=1)" AWS-->>S3Ch: Evidence (buckets enumerated / probed) S3Ch-->>TCR: Evidence alt ListBuckets failed (gate) TCR->>TCR: "gate_failed = True" Note over TCR: Step 2: GetMetrics skipped (ConnectionNotEstablished) else ListBuckets passed Note over TCR: Step 2: GetMetrics (non-mandatory) TCR->>S3Ch: get_metrics() S3Ch->>AWS: "cloudwatch.list_metrics(Namespace=AWS/S3)" AWS-->>S3Ch: Evidence (metrics count) S3Ch-->>TCR: Evidence end TCR-->>BC: TestConnectionResult BC->>BC: self.close() BC-->>UC: TestConnectionResult%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UC as User/Automation participant BC as BaseConnection.test_connection() participant S3C as S3Connection.checks() participant S3Ch as S3Checks participant TCR as TestConnectionRunner participant AWS as AWS (S3 / CloudWatch) UC->>BC: test_connection(metadata, workflow) BC->>S3C: checks() S3C-->>BC: "S3Checks(connect=lambda: self.client, bucket_names=...)" BC->>TCR: run(metadata, workflow) TCR->>TCR: fetch definition (s3.json steps) Note over TCR: Step 1: ListBuckets (ConnectionGate) TCR->>S3Ch: check_buckets() S3Ch->>AWS: "list_buckets() or list_objects(Bucket, MaxKeys=1)" AWS-->>S3Ch: Evidence (buckets enumerated / probed) S3Ch-->>TCR: Evidence alt ListBuckets failed (gate) TCR->>TCR: "gate_failed = True" Note over TCR: Step 2: GetMetrics skipped (ConnectionNotEstablished) else ListBuckets passed Note over TCR: Step 2: GetMetrics (non-mandatory) TCR->>S3Ch: get_metrics() S3Ch->>AWS: "cloudwatch.list_metrics(Namespace=AWS/S3)" AWS-->>S3Ch: Evidence (metrics count) S3Ch-->>TCR: Evidence end TCR-->>BC: TestConnectionResult BC->>BC: self.close() BC-->>UC: TestConnectionResultReviews (4): Last reviewed commit: "list_buckets: cap count locally instead ..." | Re-trigger Greptile