feat(contract-toolkit): gate credential arg on hasCredentialConfig#1958
Draft
prateek11rai wants to merge 1 commit into
Draft
feat(contract-toolkit): gate credential arg on hasCredentialConfig#1958prateek11rai wants to merge 1 commit into
prateek11rai wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
The extract-node's `args.credential = "{{credential}}"` placeholder was
injected unconditionally (NativeApp.pkl:2587). The existing
`hasCredentialConfig` flag (line 627) controlled whether the credential
configmap JSON file was emitted, but NOT whether the manifest arg was
injected. Apps that opt out of credentials (e.g. system-type apps that
read from object store rather than connecting to a partner system) got
a phantom `credential` placeholder in their manifest that resolves to
null at runtime and pollutes the contract.
Fix: wrap the assignment in `when (hasCredentialConfig) { ... }`.
Apps with `hasCredentialConfig = false` get a clean args mapping.
Default behavior preserved — connector apps that don't override
`hasCredentialConfig` see no change.
Tests
- tests/credential_gating_test.pkl — 3 facts.
- Fixtures: native_app_with_credential.pkl (regression guard for the
default) and native_app_without_credential.pkl (new behavior).
pkl test tests/*.pkl — 75/75 tests pass, 262/262 asserts.
Surfaced while bringing up atlanhq/atlan-bridge-app on the toolkit.
Bridge is a system-type app that takes no partner credentials and was
getting a stray credential arg in its generated manifest.
Tracks PART-1123.
Contributor
|
Docs reminder:
Per toolkit convention, public PKL changes should update:
Ignore this reminder only when the source change is internal and has no author-facing impact. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The extract-node's
args.credential = \"{{credential}}\"placeholder (NativeApp.pkl:2587) was injected unconditionally. The existinghasCredentialConfigflag (line 627) controlled whether the credential configmap JSON was emitted, but NOT whether the manifest arg was injected. Apps that opt out of credentials still got a phantom `credential` placeholder in their manifest.This PR adds a one-line gate.
Change
Apps with
hasCredentialConfig = falseget a clean args mapping. Default behavior preserved — connector apps that don't overridehasCredentialConfigsee byte-identical manifest output.Why this matters
Surfaced while bringing up
atlanhq/atlan-bridge-app— atype: systemapp that translates partner extracts rather than connecting to a partner source system, so it has no partner credentials. Bridge declareshasCredentialConfig = falseand was correctly skipping credential configmap generation, but its manifest still carried the phantom `credential` arg.The fix is independently useful for any app declaring
hasCredentialConfig = false— utility apps, system apps, future shared infrastructure apps.What's NOT in this PR
An earlier draft of this PR also added a configurable
extractNodeKeyto rename the manifest's first DAG node from\"extract\"to a domain-fitting key. After discussion: bridge's manifest is reference-only (never executed by AE — see ADR-0007 in atlan-bridge-app), so the node-key rename is cosmetic. Callers copy nodes into their own manifest where they can name them whatever they want.The right long-term shape for system-type apps is a dedicated first-class system-app node type in the toolkit — separate from
NativeApp.pkl's connector-shaped flow. That work is tracked separately and not in scope here. TheextractNodeKeyworkaround would have been undone when the proper node type lands; better not to ship it at all.The credential gating in this PR is the durable piece — it stays regardless of the long-term system-app node decision.
Verification
pkl test tests/*.pkl→ 75/75 tests pass, 262/262 asserts (existing tests untouched; 3 new facts added).Tests:
tests/credential_gating_test.pkl— 3 facts asserting credential-arg presence / absence based onhasCredentialConfigtests/fixtures/native_app_with_credential.pkl— defaults preserved (regression guard)tests/fixtures/native_app_without_credential.pkl— opt-out case exercisedEnd-to-end verified against
atlanhq/atlan-bridge-app—hasCredentialConfig = falsenow produces a manifest with no `credential` arg.Files touched
contract-toolkit/src/NativeApp.pkl(+3 −1)contract-toolkit/tests/credential_gating_test.pkl(new)contract-toolkit/tests/fixtures/native_app_with_credential.pkl(new)contract-toolkit/tests/fixtures/native_app_without_credential.pkl(new)Linear
Tracks PART-1123. Sub-issue of PART-737.