Skip to content

test(glue): cover KMS property forwarding - #3478

Merged
kevinjqliu merged 1 commit into
apache:mainfrom
blackmwk:test-glue-kms-property-forwarding
Oct 16, 2026
Merged

kevinjqliu merged 1 commit into
apache:mainfrom
blackmwk:test-glue-kms-property-forwarding

Conversation

@blackmwk

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow-up to the review comments on #3102. Relates to #3096.

What changes are included in this PR?

Pin the contract that Glue forwards the full input map to its KMS factory, including URI, catalog ID, warehouse, and custom properties. A recording factory returns an error before AWS client initialization, covering both the property handoff and error propagation. Warehouse validation failures identify the missing or empty case.

The existing GlueCatalog debug output excludes the raw property map, so AWS secret keys and session tokens are no longer printed.

Are these changes tested?

Using the pinned nightly-2026-04-16 toolchain:

  • cargo test --locked -p iceberg-catalog-glue --lib — 20 passed.
  • cargo clippy --locked -p iceberg-catalog-glue --all-targets -- -D warnings.
  • cargo fmt --all -- --check and git diff --check.

AI Disclosure

OpenAI Codex assisted with the tests and documentation. Review focus: the recording factory deliberately aborts initialization, so coverage is limited to the KMS property handoff and error propagation.

Record the full map received by the KMS factory and check initialization error propagation. Label missing and empty warehouse cases and document the crate-private Glue properties.

Follow-up to apache#3102.

Generated-by: Codex

@xanderbailey xanderbailey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice.

@comphead comphead left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @blackmwk makes sense

Btw you may consider AsRef interface

for

properties: &HashMap<impl AsRef<str>, impl AsRef<str>>

to avoid to_string() conversion everywhere

like

                GLUE_CATALOG_PROP_URI.to_string(),
                "http://localhost".to_string(),

@kevinjqliu
kevinjqliu added this pull request to the merge queue Oct 16, 2026
Merged via the queue into apache:main with commit 8b59642 Oct 16, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants