Emit `` instead of `{{% examples %}}` #1689

guineveresaenger · 2024-02-21T04:38:19Z

IMPORTANT This cannot be merged until the bridge has updated pulumi/pulumi to a version that includes these changes.

After that, merging will be safe.

Note: pulumi/pulumi#15475 needs to be merged first for these changes to be safe.

This PR aims to remove the nested "Example Usage" logic from the bridge, as discussed here.

The issue this is aimed at fixing is pulumi/pulumi-aws#2624 but there should be a myriad of other issues that will be addressed by this change.

In a nutshell, this PR removes any {{% example(s) }} injection from the Description fields of a resource and instead detects any and all code conversions from TF/HCL --> pulumi languages, and surrounds them with an HTML comment.
This is currently

const (
	startPulumiCodeChooser = "<!--Start PulumiCodeChooser -->"
	endPulumiCodeChooser   = "<!--End PulumiCodeChooser -->"
)

Marking up any converted code blocks using the new format will serve only as a marker for registry docsgen to surround this converted code with a language chooser.

I refrained from going full Markdown parser because I believe it would be an even larger rewrite. Instead, we detect code fences in the docstring input, and append text and converted code section to the output, with the following caveats:

If a code block is part of a Section (i.e. has a Markdown header), if we encounter errors in code conversion, the new functionality strips the entire section automatically. Non-headered code blocks with such an error will also continue to be stripped.
Using indices rather than line splitting allows us to get rid of the awkward <break> placeholders for the Import section.
The Import section no longer needs special handling in convertExamplesInner: it is valid code and will be retained as such
With a little bit of extra parsing of the code fence syntax highlighter, we can capture a few more cases of perfectly valid json or yaml that we have elided previously as "failure to convert from TF".
With the removal of examples, titles (which never actually made it into the schema) are now obsolete. We will refer to resource paths when logging conversion failures. We have done so all along when there was no exampleTitle, and this change affects only the wording of logging output.
The isFrontMatter variable was removed as well. To the best of my knowledge, Hugo front matter does not get piped through the bridge, and is generated by the registry instead.
Handwritten docs continue to bypass convertExamplesInner, with an added caveat that both third party and our own docs may add the old example shortcodes. This change will allow us to remove some of these. Registry docsgen will continue to support the shortcode way of doing things, since native providers conform to that expectation separately.

VenelinMartinov

Looks great! Thanks for waddling through this and making the changes!

I had a few questions to help me understand the code and I think it'd be great if we can split up the new parsing logic into a few functions to make it easier to understand!

Also, do you think it'd make sense to add a few more regression tests? Maybe from some non-aws providers or some of the more visited docs pages?

Lastly, do the new HTML comments require a PR somewhere else before we can ship this?

pkg/tfgen/docs.go

VenelinMartinov · 2024-02-21T14:07:37Z

pkg/tfgen/docs.go

-		// to reconvert them. But we need to surround them in the examples shortcode for rendering on the registry
+		// to reconvert them.
+		//
+		//TODO: This only works if the incoming docs already have an {{% example }} shortcode, and if they are


Does this introduce a regression in this PR? Or is this a TODO for the future?

This is not a regression. This is an issue where third party (and internal) docs writers are working with the current status quo and marking up their docs with shortcodes.
See here for an internal example.
The TODO here is to write code to handle expected legacy behavior on users' behalf, or ask them to migrate.
Currently, the registry will continue to accept old shortcode behavior, as many native providers expect to use them.

Can you open an issue for this and link it with the TODO.

VenelinMartinov · 2024-02-21T14:14:33Z

pkg/tfgen/docs.go

+				inCodeBlock = false
+			}
+		} else {
+			// Keep track of header locations. These should never be inside code blocks.


Should we add an error if this assumption is violated?

What I was trying to say was "we never look for a header inside a code block" - describing behavior, not assumption.
The new code is a bit more clear - we never look for a header if we are inside a code block.

pkg/tfgen/docs.go

VenelinMartinov · 2024-02-21T14:23:08Z

pkg/tfgen/docs.go

-		header, wroteHeader := section[0], false
-		isFrontMatter, isExampleUsage := !strings.HasPrefix(header, "## "), header == "## Example Usage"
+	// Now we know where the headers and code fences are.
+	textStart := 0


There's quite a few interactions and context one needs to keep when reading here, would this also make sense to be split into a few functions?

pkg/tfgen/test_data/convertExamples/aws_lambda_function_out.md

VenelinMartinov · 2024-02-21T14:31:59Z

pkg/tfgen/docs.go

-				if output.Len() > 0 {
-					fprintf(output, "\n")
+			} else {
+				// Take already-valid code blocks as-is.


Is this covered by a regression test?

VenelinMartinov · 2024-02-21T14:36:33Z

Okay, I think you've opened pulumi/pulumi#15475 to parse the new HTML comments, great!

t0yv0 · 2024-02-21T16:05:52Z

pkg/tfgen/test_data/address_map/expected.json

@@ -25,5 +25,5 @@
    "ips": "The set of IPs on the Address Map.",
    "memberships": "Zones and Accounts which will be assigned IPs on this Address Map."
  },
-  "Import": "## Import\n\n```sh\u003cbreak\u003e $ pulumi import MISSING_TOK example \u003caccount_id\u003e/\u003caddress_map_id\u003e \u003cbreak\u003e```\u003cbreak\u003e\u003cbreak\u003e"
+  "Import": "## Import\n\n```sh\n$ pulumi import MISSING_TOK example \u003caccount_id\u003e/\u003caddress_map_id\u003e\n```\n\n"


Remaining issue with MISSING_TOK? Should that be an error instead?

I'm happy to file an issue here but it's out of scope for this change I think!

pkg/tfgen/test_data/convertExamples/wavefront_dashboard_json_out.md

pkg/tfgen/docs.go

VenelinMartinov

lgtm

iwahbe

LGTM

This pull request depends on corresponding changes in the pulumi-terraform bridge. The bridge will mark up each code section it finds (not just ones found under an "Example Usage" header) with an HTML comment - currently this will be `` and `` but the name is subject to change. In this PR, we are adding logic to detect if an incoming Description block has an `{{% examples }}` shortcode markup: if yes, we will process it as before. If no, we will switch to new functionality that relies on documentation with code sections having no short codes, no "Examples Section" logic, and will generate every code section it finds in an incoming schema that is in the Description/Content section of a registry page with a chooser and language choosables. This work is part of pulumi/pulumi-aws#2624. Edit: unit tests are added This PR is a prerequisite for pulumi/pulumi-terraform-bridge#1689. It aims to support both legacy and new behavior.  # Description  Fixes # (issue) ## Checklist - [x] I have run `make tidy` to update any new dependencies - [x] I have run `make lint` to verify my code passes the lint check - [x] I have formatted my code using `gofumpt`  - [x] I have added tests that prove my fix is effective or that my feature works  - [ ] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Cloud API version

VenelinMartinov · 2024-02-26T12:12:49Z

Is this blocked by #1699? From the description there it looks like we'd be regressing in some providers if we merge this.

t0yv0 · 2024-02-28T14:48:15Z

Note: pulumi/pulumi#15475 needs to be merged first for these changes to be safe.

I don't know about 1699.

iwahbe · 2024-02-28T16:00:19Z

Note: pulumi/pulumi#15475 needs to be merged first for these changes to be safe.

I don't know about 1699.

I don't believe that it is blocked on #1699.

VenelinMartinov · 2024-02-28T16:14:25Z

Yup, I confirmed it with the docs team - #1699 is not a blocker but the bridge has yet to pick up pulumi/pulumi#15475

…s also

…re they are written in. Still needs: how have we been adding short codes to hand written docs?

…r the TF markdown with less nesting and fewer tracking variables. This also removes the need for <break> placeholders in the import section processing. However, every Description entry now ends with a double newline and I'm not yet sure why.

…now. At least it's consistent.

… Adjust tests.

…ructs. Add tests. Rename some consts and vars. Remove need for writeTrailingNewline.

Test changes are due to HCL->PCL improvements. The do not effect the new code AFAIK.

VenelinMartinov

lgtm

codecov · 2024-03-07T15:05:34Z

Codecov Report

Attention: Patch coverage is 70.81081% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 59.19%. Comparing base (b364656) to head (532a693).

Files	Patch %	Lines
pkg/tfgen/docs.go	68.78%	53 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1689      +/-   ##
==========================================
- Coverage   59.75%   59.19%   -0.56%     
==========================================
  Files         301      308       +7     
  Lines       42046    42471     +425     
==========================================
+ Hits        25123    25141      +18     
- Misses      15490    15903     +413     
+ Partials     1433     1427       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

guineveresaenger requested review from iwahbe and a team February 21, 2024 04:38

VenelinMartinov reviewed Feb 21, 2024

View reviewed changes

t0yv0 reviewed Feb 21, 2024

View reviewed changes

iwahbe reviewed Feb 21, 2024

View reviewed changes

guineveresaenger requested review from iwahbe, VenelinMartinov and a team February 21, 2024 16:31

guineveresaenger mentioned this pull request Feb 22, 2024

Add ability to process docs without dedicated Examples logic pulumi/pulumi#15475

Merged

6 tasks

VenelinMartinov approved these changes Feb 23, 2024

View reviewed changes

iwahbe approved these changes Feb 23, 2024

View reviewed changes

This was referenced Feb 23, 2024

Remove Include-CloudWatch-Logging-section-in-Lambda-Example.patch pulumi/pulumi-aws#3473

Closed

Remove extraneous <break> markup from NetworkFirewallResourcePolicy pulumi/pulumi-aws#3474

Closed

guineveresaenger added the DO_NOT_MERGE label Feb 23, 2024

guineveresaenger changed the title ~~Refactor: convertExamplesInner~~ [DO NOT MERGE] Refactor: convertExamplesInner Feb 23, 2024

This was referenced Feb 23, 2024

Replace AcmCertificateValidation examples shortcode with Pulumi Code Chooser pulumi/pulumi-aws#3476

Closed

[docs] Enable PulumiCodeChooser on nested docs descriptions pulumi/pulumi#15506

Open

guineveresaenger added 7 commits March 7, 2024 15:50

Add informational 'TODO's everywhere to map out the code. Adds some Q…

527a363

…s also

Remove isExampleUsage. Remove all examples mustaches in the areas whe…

a1879b3

…re they are written in. Still needs: how have we been adding short codes to hand written docs?

Cleanup and adding a trailing newline to every description field for …

ae12668

…now. At least it's consistent.

Parse the incoming string instead of splitting up lines

5a52297

Import Section now can be handled the same as all other sections

5f64107

cleanup

b0b4735

guineveresaenger and others added 19 commits March 7, 2024 15:50

have out.md with fiddly bits

f9d5974

mostly works but weird whitespace

7b0fa29

Add test case for convertDocsInner

33b8d5c

Fix up imports parsing to no longer use lines and placeholder breaks.…

cd01b91

… Adjust tests.

Remove the placeholder HTML breaks from all other tests as well

069632a

handle h3 vs h2

e6431ce

remove stripSubsectionsWithErrors as unnecessary

ac42f63

clean up exampleTitle as well

6ca0569

clean up TODO

5f60767

code review: Refactor location parsing into separate functions and st…

53d8276

…ructs. Add tests. Rename some consts and vars. Remove need for writeTrailingNewline.

remove print statement

7e6aecd

Ensure installation of AWS plugin

5f8f46b

try to add the Lambda test to the general TestConvertExamples suite

420c3d4

AWS version

c92c853

Use correct AWS plugin version

9695828

move AWS test back to convertExamples

0382d47

specify archive version also

9ba5031

skip newline tests on windows

f4a33e0

fix rebase and regen tests

87f8e4f

Test changes are due to HCL->PCL improvements. The do not effect the new code AFAIK.

iwahbe force-pushed the guin/missing-examples branch from 8c29d7f to 87f8e4f Compare March 7, 2024 14:57

VenelinMartinov approved these changes Mar 7, 2024

View reviewed changes

fix lint

532a693

iwahbe changed the title ~~[DO NOT MERGE] Refactor: convertExamplesInner~~ Emit  instead of {{% examples %}} Mar 7, 2024

iwahbe removed the DO_NOT_MERGE label Mar 7, 2024

iwahbe merged commit cf13280 into master Mar 7, 2024
9 checks passed

iwahbe deleted the guin/missing-examples branch March 7, 2024 16:21

iwahbe mentioned this pull request Mar 8, 2024

Revert schema path changes #1744

Merged

sean1588 mentioned this pull request Mar 18, 2024

Language chooser is missing on Registry docs pages #926

Closed

guineveresaenger mentioned this pull request Mar 22, 2024

SyntheticsTest docs is missing language chooser pulumi/pulumi-datadog#242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit `` instead of `{{% examples %}}` #1689

Emit `` instead of `{{% examples %}}` #1689

guineveresaenger commented Feb 21, 2024 •

edited

VenelinMartinov left a comment

VenelinMartinov Feb 21, 2024

guineveresaenger Feb 21, 2024

iwahbe Feb 23, 2024

VenelinMartinov Feb 21, 2024

guineveresaenger Feb 22, 2024

VenelinMartinov Feb 21, 2024

t0yv0 Feb 21, 2024

VenelinMartinov Feb 21, 2024

VenelinMartinov commented Feb 21, 2024

t0yv0 Feb 21, 2024

guineveresaenger Feb 22, 2024

VenelinMartinov left a comment

iwahbe left a comment

VenelinMartinov commented Feb 26, 2024 •

edited

t0yv0 commented Feb 28, 2024

iwahbe commented Feb 28, 2024

VenelinMartinov commented Feb 28, 2024 •

edited

VenelinMartinov left a comment

codecov bot commented Mar 7, 2024 •

edited

Emit  instead of {{% examples %}} #1689

Emit  instead of {{% examples %}} #1689

Conversation

guineveresaenger commented Feb 21, 2024 • edited

VenelinMartinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov left a comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

VenelinMartinov commented Feb 26, 2024 • edited

t0yv0 commented Feb 28, 2024

iwahbe commented Feb 28, 2024

VenelinMartinov commented Feb 28, 2024 • edited

VenelinMartinov left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2024 • edited

Codecov Report

Emit `` instead of `{{% examples %}}` #1689

Emit `` instead of `{{% examples %}}` #1689

guineveresaenger commented Feb 21, 2024 •

edited

VenelinMartinov commented Feb 26, 2024 •

edited

VenelinMartinov commented Feb 28, 2024 •

edited

codecov bot commented Mar 7, 2024 •

edited