Refactor AWS resource configuration and token mappings - #6601
Conversation
Summary: - Keep resources.go as the provider entrypoint and move distinct config, token, override, alias, metadata, autonaming, and ID logic into focused files. - Use mapped module inference with fallback, remove the Plugin Framework registration special case, and prune redundant explicit token mappings. - Preserve legacy aliases and non-default public tokens, with tests that reject stale or unnecessary overrides. Rationale: - Provider assembly no longer needs to carry thousands of lines of mapping details, while existing SDK paths and resource URNs remain compatible. - Automatic token computation reduces upgrade maintenance without changing curated AWS module and member names. Tests: - make test_provider - make schema - make lint (blocked: installed golangci-lint was built with Go 1.25, but the provider targets Go 1.26.5)
Does the PR have any schema changes?Generated by schema-tools v0.8.1. Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6601 +/- ##
=======================================
Coverage 15.84% 15.84%
=======================================
Files 376 376
Lines 92183 92183
=======================================
Hits 14608 14608
Misses 77554 77554
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review Verdict: ApprovedVerified this is a behavior-preserving mechanical refactor: every one of the 843 resource and 405 data-source entries pruned from resources.go was confirmed (programmatically) to contain nothing but a redundant Tok field, the split-out files are byte-identical code moves, go build/vet/test all pass locally including new regression tests, and schema-tools reports zero breaking changes. Automated low-risk assessment, not a human review. View session · Was this review helpful? Yes · No |
![eon-pulumi-agent[bot]](https://avatars.githubusercontent.com/in/4080224?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff (AGENTS.md, CONTRIBUTING.md, docs/provider-runtime.md, and all provider/*.go files) plus four parallel specialist passes (correctness, security, AGENTS.md/REVIEW.md compliance, tests).
What I verified directly, beyond reading the diff:
go build .,go vet ., and the fullgo test .provider suite pass locally at HEAD (including the two new regression testsTestExplicitTokenMappingsAreNecessaryandTestAutomaticTokenMappings).- Programmatically extracted every resource/data-source map entry from
master's monolithicprovider/resources.goand diffed the key sets against the newprovider/resource_overrides.go/provider/data_source_overrides.go. Of the 843 resource and 405 data-source entries that no longer appear as explicit map entries, every single one consisted of nothing but aTok:field inmaster— none carriedFields,Docs,Aliases,DeprecationMessage, or other customization that could have been silently dropped during the prune. - Diffed
aliases.go,autonaming.go,compatibility.go,computed_ids.go,resource_metadata.go, and the credential-validation logic inconfig.goagainst their prior inline/closure form inmaster— all are byte-identical code moves (rAliaswas renamed toaddLegacyAlias, otherwise unchanged). make schema's schema-tools output (linked in the PR body) reports no breaking changes and no new/removed resources or types, which independently corroborates that the token-strategy switch (tks.MappedModules→fallbackstrat.MappedModulesWithInferredFallback) preserved every existing token.
Findings: no Important issues. Two minor Nits below (neither blocks approval).
Required CI check (Sentinel) is green. The unrelated upstream_tests (sqs) job failure visible in the checks list is not part of the required check set and pre-dates this PR's scope, so I did not weigh it against this change.
| } | ||
|
|
||
| strategy, err := fallbackstrat.MappedModulesWithInferredFallback( | ||
| prov, "aws_", "", moduleMap, finalize) |
There was a problem hiding this comment.
Nit — tests
The switch to fallbackstrat.MappedModulesWithInferredFallback adds a genuine new capability (inferred-module fallback for TF prefixes not in moduleMap), but no current resource actually exercises that fallback path today — every previously-unmapped resource either has an explicit Tok override in resourceOverrides()/dataSourceOverrides(), a legacy alias token, or (as of this PR) a new moduleMap entry. TestAutomaticTokenMappings only covers module-mapped cases, so the inference path itself has zero positive or negative test coverage.
Consider a small unit test (in the style of the bridge's own fallbackstrat_test.go) that feeds awsTokenStrategy a synthetic TF resource name with a prefix absent from moduleMap, to lock in the finalize closure's behavior (including the name == "" guard) for the day a future upstream resource actually needs the fallback.
Why this matters
This is the actual new behavior this refactor was introduced to enable, but it is currently unverified — a future regression in the fallback path (e.g. a bad finalize transform) would not be caught until a real upstream resource hit it in production.
| "backup": backupMod, | ||
| "batch": batchMod, | ||
| "bedrock_foundation": bedrockFoundationMod, | ||
| "bedrock_model": bedrockModelMod, |
There was a problem hiding this comment.
Nit — logic
moduleMap gains three new entries in this PR: "vpc" → vpcMod ("Vpc"), "auditmanager" → auditmanagerMod ("Auditmanager"), and "resourceexplorer2" → "ResourceExplorer". Only the last one is load-bearing today — it corrects a casing mismatch the naive inferred/PascalCase strategy would otherwise produce ("Resourceexplorer2" instead of "ResourceExplorer"). The vpc and auditmanager entries currently produce the same token whether or not they're in moduleMap, since InferredModules (the fallback path) would derive the same PascalCase name for those prefixes.
Consider a short comment distinguishing "kept because it corrects the auto-derived name" (like resourceexplorer2) from "kept for explicitness/determinism even though the fallback would already produce the same result" (like vpc/auditmanager), so a future reader doesn't have to re-derive which entries are actually load-bearing.
|
This PR has been shipped in release v7.42.0. |
2 participants
Summary
provider/resources.gohad grown past 6,000 lines and mixed provider assembly with configuration, token policy, overrides, aliases, metadata, autonaming, and computed IDs.resources.goas the conventional entrypoint, move cohesive behavior into focused files, use mapped-module inference with fallback, remove the Plugin Framework registration special case, and prune 1,418 redundant token declarations.The remaining explicit mappings preserve published module or member names. The audit found 199 token-only mappings; each still exists upstream and differs from the automatic token. Legacy resource aliases remain unchanged.
Change Type
provider/)Validation Evidence
make lint— blocked because the installedgolangci-lintwas built with Go 1.25 while this repository targets Go 1.26.5.make test_providermake schemamake build_sdkswas not run because schema generation produced no token or metadata changes requiring SDK regeneration.Command output snippets
Risk
Rollback