Skip to content

Fixes #30466: ingest S3 bucket-level and root-level object tags#30467

Open
akashverma0786 wants to merge 5 commits into
mainfrom
fix/s3-connector-improvement
Open

Fixes #30466: ingest S3 bucket-level and root-level object tags#30467
akashverma0786 wants to merge 5 commits into
mainfrom
fix/s3-connector-improvement

Conversation

@akashverma0786

@akashverma0786 akashverma0786 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Fixes #30466

What

The S3 connector was dropping two categories of S3 tags. This adds both.

  • Root-level object tags_get_bucket_name_and_key used len(parts) > 2, so a file at the bucket root (s3://bucket/file.csv, 2 segments) returned (None, None) and its object tags were skipped. Changed to >= 2.
  • Bucket-level tags — added get_bucket_tagging for the bucket container (routed via a new _fetch_s3_tags helper: object tags for leaf files, bucket tags for the bucket). The bucket container was also missing its container_fqn, which is now set so its tags can attach.

Both confirmed against DataHub's S3 source (get_key_prefix, use_s3_bucket_tags).

Not changed

Nested-object tagging, folder containers (no tag leak), exception handling, and the includeTags=false default are all byte-identical to before.

Testing

  • Unit: 293 passed (added root-level path test + bucket container_fqn assertions)
  • Integration (real MinIO): 24 passed in tests/integration/s3/
  • Live end-to-end against MinIO: bucket tags, root-object tags, and nested-object tags all land; includeTags: true required.

Greptile Summary

Adds S3 bucket-level and root-object tag ingestion.

  • Routes leaf containers to object-tag retrieval and bucket containers to bucket-tag retrieval.
  • Assigns bucket container FQNs so retrieved tags can attach to bucket entities.
  • Expands unit coverage for path parsing, tag routing, FQN assignment, and exception handling.

Confidence Score: 5/5

The PR appears safe to merge with no blocking failures remaining.

No blocking failures remain.

Important Files Changed

Filename Overview
ingestion/src/metadata/ingestion/source/storage/s3/metadata.py Adds root-object and bucket tag routing and assigns FQNs to generated bucket containers.
ingestion/tests/unit/topology/storage/test_s3_methods.py Adds focused coverage for root-level path parsing, tag API selection, bucket FQNs, and exception behavior.
ingestion/tests/unit/topology/storage/test_s3_storage.py Updates unstructured bucket-container expectations to include the generated FQN.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[S3 container details] --> B{Leaf container with bucket and key?}
    B -->|Yes| C[Fetch object tags]
    B -->|No| D{Bucket-root path?}
    D -->|Yes| E[Fetch bucket tags]
    D -->|No| F[Return no tags]
    C --> G[Associate tags using container FQN]
    E --> G
Loading

Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/s3-connecto..." | Re-trigger Greptile

- _get_bucket_name_and_key: >2 -> >=2 so root-level files (s3://bucket/file.csv)
  no longer skip their object tags
- add get_bucket_tagging for the bucket container via _fetch_s3_tags; set
  container_fqn on the bucket container so its tags can attach
- nested-object tagging and all other behavior unchanged; unit tests updated
@akashverma0786
akashverma0786 requested a review from a team as a code owner July 24, 2026 11:12
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

✅ PR checks passed

The linked issue has a description and all required Shipping project fields set. Thanks!

@github-actions

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@akashverma0786 akashverma0786 added Ingestion safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch labels Jul 24, 2026
Comment thread ingestion/src/metadata/ingestion/source/storage/s3/metadata.py Outdated
Adds TestFetchS3Tags (bucket vs object routing, empty tagsets, None path,
leaf-at-root fallback, multi-tag) and TestYieldContainerTags (missing
container_fqn guard, S3 exception swallowed) for the new bucket-tag path.
@akashverma0786
akashverma0786 enabled auto-merge (squash) July 24, 2026 11:24
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

🔴 Playwright Results — workflow failed

Validated commit d8ce1a4cd03087ff17ef4083af8ba35c513b442a in Playwright run 30241300559, attempt 1.

✅ 104 passed · ❌ 2 failed · 🟡 1 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ❌ unmet · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 50m 50s

⏱️ Max setup 3m 7s · max shard execution 14m 47s · max shard-job elapsed before upload 22m 22s · reporting 7s

🌐 200.90 requests/attempt · 1.76 app boots/UI scenario · 0.00% common-shard skew

Optimization targets still in progress:

  • Browser traffic was 200.9 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 1.76 per UI scenario (222 boots / 126 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 46 0 0 3 0 0
✅ Shard ingestion-01 28 0 0 0 0 0
🔴 Shard ingestion-02 30 2 1 0 0 0

Genuine Failures (failed on all attempts)

Features/TestSuiteMultiPipeline.spec.tsTestSuite multi pipeline support (shard ingestion-02)
Error: locator.click: Error: strict mode violation: getByText('Data Observability') resolved to 2 elements:     1) <div class="d-flex justify-between gap-1">Data Observability</div> aka getByTestId('profiler').getByText('Data Observability')     2) <span class="tw:truncate tw:text-xs tw:font-medium tw:text-primary">Data Observability</span> aka getByTestId('asset-health-row-dataObservability').getByText('Data Observability')  Call log: �[2m  - waiting for getByText('Data Observability')�[22m 
Features/TestSuiteMultiPipeline.spec.tsEdit the pipeline's test case (shard ingestion-02)
Error: locator.click: Error: strict mode violation: getByText('Data Observability') resolved to 2 elements:     1) <div class="d-flex justify-between gap-1">Data Observability</div> aka getByTestId('profiler').getByText('Data Observability')     2) <span class="tw:truncate tw:text-xs tw:font-medium tw:text-primary">Data Observability</span> aka getByTestId('asset-health-row-dataObservability').getByText('Data Observability')  Call log: �[2m  - waiting for getByText('Data Observability')�[22m 
🟡 1 flaky test(s) (passed on retry)
  • Features/IncidentManager.spec.tsComplete Incident lifecycle with table owner (shard ingestion-02, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@gitar-bot

gitar-bot Bot commented Jul 27, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Adds S3 bucket-level and root-level object tag ingestion with associated routing and FQN fixes, addressing the untested new bucket-tag branch finding. No issues found.

✅ 1 resolved
Quality: New bucket-tag branch of _fetch_s3_tags is untested

📄 ingestion/src/metadata/ingestion/source/storage/s3/metadata.py:242-254
The added tests cover the root-level object path (_get_bucket_name_and_key) and the bucket container_fqn, but nothing exercises _fetch_s3_tags or its new get_bucket_tagging else-branch. The bucket-level tag ingestion — the core behavior this PR adds — has no unit coverage, so a regression (e.g. wrong path routing or a broken get_bucket_tagging call) would pass CI. Add a unit test that mocks s3_client.get_bucket_tagging and asserts bucket-container tags are returned, plus a case confirming a folder container yields no tags.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ingestion safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S3 connector improvement: ingest bucket-level tags and root-level object tags

1 participant