tfgen: fix Import sections for GCloud IAM-style resources - #3586
tfgen: fix Import sections for GCloud IAM-style resources#3586jkodroff wants to merge 3 commits into
Conversation
Upstream documents a family of IAM resources on a single page, using
quoted, space-delimited composite IDs. Four defects compounded there, so
that a page like gcp:projects/iAMMember rendered no usable import
guidance at all.
parseImportCode's ID capture was a single whitespace-free token, so a
quoted ID containing spaces never matched and the `terraform import`
line was emitted verbatim. Widen the capture to accept a quoted string,
keeping the quotes in the emitted command.
That alone would make things worse: every example on a shared page would
then parse, and each would be stamped with the token of the page being
generated - so the IamMember example on the IamBinding page would become
a silently wrong `pulumi import`. Resolve each example's own Terraform
resource type through the provider's resource map instead, falling back
to the page's token when the type is not one this provider bridges.
Upstream also embeds `terraform import` examples in prose, outside any
fence. Rewrite those code spans too.
Finally, drop Terraform-only HCL `import { ... }` blocks whichever way
their fence is tagged. Only ```terraform was recognized, but upstream
overwhelmingly writes ```tf. Leaving one in did more than render stray
HCL: isHCL treats tf/hcl as convertible, `pulumi convert` rejects import
blocks, and convertExamples then strips the entire enclosing subsection.
That is why the Import body vanished on pages whose every subsection
opens with such a block.
Fixes #3584
Fixes #3585
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review Verdict: ApprovedFixes a real, well-documented parsing bug (space-delimited quoted IAM import IDs, sibling-resource token misattribution, prose-embedded terraform import commands, and tf/hcl-fenced import blocks that were silently deleting whole doc subsections), backed by 30+ pinned regex test cases plus a real upstream testdata fixture whose output I verified matches the PR's claims exactly. All required CI checks are green; four parallel specialist passes (correctness, security, tests, compliance) found no issues above the confidence floor. Automated low-risk assessment, not a human review. View session · Was this review helpful? Yes · No |
![unblocked[bot]](https://avatars.githubusercontent.com/in/166219?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No issues found
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
Scope the importTokens doc comment to Google Cloud, which is the only provider observed to share one upstream page across an IAM triad, and correct the copyright year on the new test file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3586 +/- ##
==========================================
+ Coverage 70.47% 70.50% +0.02%
==========================================
Files 358 358
Lines 39474 39501 +27
==========================================
+ Hits 27820 27850 +30
+ Misses 9705 9703 -2
+ Partials 1949 1948 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 parser/rewriter changes in pkg/tfgen/docs.go plus the new test file and testdata (pkg/tfgen/import_code_test.go, test_data/parse-imports/gcp-project-iam.md, and the random-string-full-expected.md golden update).
What I checked:
- Ran the full new test suite (
TestParseImportCode,TestParseImports_SpaceDelimitedIDs,TestParseImports_DropsHCLImportBlocks,TestParseImports_UnmappedResourceFallsBackToPageToken) plus the pre-existingpkg/tfgensuite — all green, no regressions. - Manually ran the parser against the included
gcp-project-iam.mdfixture and confirmed the output matches exactly what the PR description claims: every IAM-triad example gets its own resource's token, noterraform importsurvives, and no subsection is stripped. The one cosmetic leftover the author called out (a dangling preamble sentence after a droppedtfHCL block) is present and exactly as described, not something introduced beyond what's disclosed. - Traced the regex change (
importCodePattern's new quoted-ID alternation), theimportTokens.tokenForsibling-resource fallback,rewriteInlineImportCode's prose rewriting, and theisHCLFenceInfowidening (terraform/tf/hcl) against the code paths that consume them (rewriteImportFence,rewriteImportMarkdown,convertExamplesInner). Behavior for non-IAM/ordinary resources (nil or absentProviderInfo.Resources) is unchanged, matching the existing golden fixtures. - Four specialist passes (correctness, security, tests, AGENTS.md compliance) turned up no Important findings. Security found nothing exploitable (build-time-only doc rewriting over provider docs, RE2 regex engine rules out ReDoS). Compliance found no rule violations. Tests and correctness each surfaced only very low-confidence coverage nits (missing test cases for an already-narrow fallback branch, multi-span-per-line inline rewriting) that don't meet the bar for a comment on a PR this thoroughly tested.
- Confirmed the pushed follow-up commit (
tfgen: address review feedback) is a no-op comment/copyright-year tweak with no functional change.
CI: the one required check (Ensure test assets build cleanly) is green. A non-required dynamic package test (TestSchemaGenerationFullDocs/random) is failing, but that's the exact, explicitly-disclosed golden-drift the author called out in the PR description — deferred on purpose to the separate accept-goldens workflow rather than folded into this diff — and it matches a diff line the author explicitly pointed to.
No inline comments to post.
jkodroff
changed the title
The Import-section fix rewrites `terraform import` code spans that appear in prose, outside a fence. The random provider's password and string docs contain exactly those, so TestSchemaGenerationFullDocs needs its golden refreshed alongside the tfgen test_data fixture. Regenerated with `make test_accept`, whose scoped .pulumi-test plugin cache keeps converted examples identical to CI's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
guineveresaenger
left a comment
guineveresaenger
left a comment
There was a problem hiding this comment.
I think this is good, although perhaps a bit too large-brush for something that we assume affects only GCP at this point.
There's a few "more correct" tweaks in here, too, that I think other providers can benefit from; however we're trading correctness with a few other resources when we're trying to determine the correct token for each pulumi import invocation.
What's the practicality tradeoff using a docs override inside Pulumi GCP as an alternative?
| // <some-ID> | ||
| // | ||
| // The ID is either a single whitespace-free token or a quoted string. Quoting is what lets a | ||
| // composite ID contain spaces, as IAM-style resources do: |
There was a problem hiding this comment.
This may happen to non-IAM resources also. Example is good; comment should be more generic.
| // these fences `tf` far more often than `terraform`, and the distinction matters: an HCL | ||
| // block left in the Import section is handed to convertExamples, which strips the entire | ||
| // enclosing subsection when the conversion fails. | ||
| func isHCLFenceInfo(info string) bool { |
|
|
||
| // tokenFor returns the token to use for an example importing the given Terraform resource | ||
| // type, falling back to the token of the resource being documented when that type is not a | ||
| // resource this provider bridges. |
There was a problem hiding this comment.
This would not give the right token when Docs.Source is set to override to another location. For example, this would give the wrong token for aws_alb.
| // TestParseImports_UnmappedResourceFallsBackToPageToken pins the fallback: when an example | ||
| // names a Terraform resource this provider does not bridge, we keep stamping the token of the | ||
| // page being generated rather than dropping the example. |
There was a problem hiding this comment.
I think this comment is unnecessary
| // TestParseImports_DropsHCLImportBlocks covers | ||
| // https://github.com/pulumi/pulumi-terraform-bridge/issues/3585. | ||
| // | ||
| // A Terraform `import { ... }` block is Terraform-only config syntax with no Pulumi analogue, | ||
| // so it has to be dropped from the Import section. Upstream tags these fences ```tf far more | ||
| // often than ```terraform, and leaving one in does more than render stray HCL: isHCL treats | ||
| // tf/hcl as convertible, the conversion cannot succeed, and convertExamples then strips the | ||
| // entire enclosing subsection - heading, prose and sibling examples included. |
There was a problem hiding this comment.
Can the test name just be TestParseImports_DropsHCLImportBlocksForAllTerraformLanguageIdentifiers, and have no comment?
| } | ||
|
|
||
| // inlineImportCodeSpan matches a backtick-delimited code span in prose. | ||
| var inlineImportCodeSpan = regexp.MustCompile("`[^`]*`") |
There was a problem hiding this comment.
I think it would be great to find a name for this variable that doesn't conflate "code" and "CLI command". We're not looking for a thing of code; we're looking for a shell command.
I'm pointing this out because we do look for real code blocks elsewhere in this file and it would be good to differentiate.
2 participants
Fixes #3584. Fixes #3585.
Upstream documents a family of IAM resources on a single page, using quoted, space-delimited composite IDs. Four defects compounded there, so a page like
gcp:projects/iAMMemberrenders no usable import guidance at all — the entire Import body is two->notes, one of which itself contains a bareterraform importcommand.What changed
1. Quoted, space-delimited IDs.
parseImportCode's ID capture was([^\s+])— one whitespace-free token. IAM import IDs are quoted and space-delimited, so the pattern never matched andrewriteImportLinesleft the upstream line verbatim. The capture now also accepts a quoted string, keeping the quotes in the emitted command.2. Sibling-resource tokens. Fixing (1) alone would have made things worse. Upstream ships one page for the whole triad, so once every example parses, every example gets stamped with the token of the page being generated — the
IamMemberexample on theIamBindingpage would silently become a wrongpulumi import. Each example's own Terraform resource type is now resolved throughProviderInfo.Resources, falling back to the page's token when the type is not a resource this provider bridges. That fallback is what keeps every non-sibling case byte-identical to today.3. Import examples in prose. Upstream embeds
terraform importin backticked spans outside any fence. Those are rewritten now too.4.
tf/hcl-fenced HCLimport {}blocks (#3585, found while verifying (1)–(3) against the live page). Only```terraformwas recognized for dropping, but upstream overwhelmingly writes```tf. Leaving one in did more than render stray HCL —isHCLtreatstf/hclas convertible,pulumi convertrejects import blocks (Unsupported block type; Blocks of type "import" are not expected here), andconvertExamplesInnerresponds by stripping the entire enclosing subsection: heading, prose, and sibling examples. Every H3 on the gcp IAM page opens with such a block, which is why the whole body disappeared. This was invisible in local repros that setPULUMI_CONVERT=0.Result
Full parser path over upstream
google_project_iam.html.markdowngeneratingIAMMember. Before, one wrongpulumi importline and two unrewrittenterraform importlines; after:Each example carries its own resource's token, no
terraform importsurvives, and no subsection is stripped.Tests
Written failing first, then fixed.
TestParseImportCode— 30 cases pinning the regex. 24 pass on unfixed code and exist as the regression guard, including 10 "must NOT parse" cases (unquoted spaces, unterminated quote, two quoted args, trailing comment, HCLimport {}block, prose mentions). 6 were red: the quoted space-delimited IDs.TestParseImports_SpaceDelimitedIDs— the real upstream page, added as testdata.TestParseImports_UnmappedResourceFallsBackToPageToken— pins the fallback so unbridged siblings keep today's behavior.TestParseImports_DropsHCLImportBlocks— one subtest per fence tag;terraformpassed as the guard,tfandhclwere red.One golden moved, and it is an improvement:
random-string-full-expected.md, two prose lines fromterraform import random_string.test testtopulumi import random:index/string:String test test.pkg/tfgen's 4 remaining failures are pre-existing — verified identical on a clean checkout ofmainin a throwaway worktree, and unrelated (simple:resourcevssimple:Resourcecasing in example conversion).Notes for the reviewer
dynamicgoldens are deliberately untouched.TestSchemaGenerationFullDocsalready fails onmainfor both random and fortimanager from unrelated drift. I regenerated both in a worktree and diffed: this change's only effect is the same two prose lines in random, and fortimanager comes out byte-identical. Committing them would drag that unrelated drift into this PR, so I left it to the accept-goldens workflow.tffence leaves its introducing sentence dangling — "Animportblock (Terraform v1.5.0 and later) can be used…" now leads into thepulumi importexample. Fixing it needs heuristics for detecting a preamble to a dropped block, which felt like the wrong thing to fold in here. Happy to file it.🤖 Generated with Claude Code