Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Tables rendered as such strip the next header #2466

Closed
guineveresaenger opened this issue Oct 8, 2024 · 3 comments · Fixed by #2632
Closed

[docs] Tables rendered as such strip the next header #2466

guineveresaenger opened this issue Oct 8, 2024 · 3 comments · Fixed by #2632
Assignees
Labels
area/docs Improvements or additions to docs for this repo kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@guineveresaenger
Copy link
Contributor

What happened?

Trying to run registry-docs on any provider with a table element in the index.md results in missing headers.

Example

Examples: pulumi-ise, pulumi-sdwan, pulumi-newrelic.

  1. Add registryDocs: true to .ci-mgmt.yaml
  2. make ci-mgmt
  3. make tfgen && make build_registry_docs
  4. Observe:
<td>A path to a PEM-encoded certificate authority used to verify the remote agent's certificate. The `NEW_RELIC_API_CACERT` environment variable can also be used.</td>
</tr>
</tbody>
</table>
##

when it should be

 </tr>
 </tbody>
 </table>

## Authentication Requirements

Output of pulumi about

n/a; bridge at tip of default branch.

Additional context

Possibly related: teekennedy/goldmark-markdown#19

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@guineveresaenger guineveresaenger added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team area/docs Improvements or additions to docs for this repo labels Oct 8, 2024
@iwahbe iwahbe removed the needs-triage Needs attention from the triage team label Oct 9, 2024
@guineveresaenger guineveresaenger self-assigned this Oct 24, 2024
@guineveresaenger
Copy link
Contributor Author

This one's a doozy, and there's updates/modifications to this behavior.

Since #2603, which was supposed to fix this issue this behavior shows up slightly differently. Basically, any Markdown document nodes that appear after a Table element gets rendered, will not be rendered.

Example:

## Here is a section

|t1    | *t2* |
|----------|------|
| **r1c1** | r1c2 |

Some text

## Another Header

With More Stuff!

turns into

## Here is a section

|t1    | *t2* |
|----------|------|
| **r1c1** | r1c2 |

Story so far:

  1. We did not notice this for Add table renderer to render tables as Markdown. Add tests. #2603, because none of our tests verified what would happen after a Table.
  2. This behavior is seen anytime there is text after a Table element. For some providers, this was not visible, because the table is at the very end of the document.
  3. The suspicion is that the link to the next node is lost somehow when rendering but it's really unclear why. Tried exploring the following:
    • See if there's missing whitespace between the Table and the next document element (inconclusive because there's no insight into the standard renderer, and it seems impossible to add extra whitespace?)
    • Check for any interference with other custom renderers (there is none; this behavior persists regardless of which custom rendered have registered rendering functions)
    • Attempt to find break points or a panic (but again, there's not much insight into the "regular" renderer and it becomes really difficult to see where things are broken.

@mjeffryes mjeffryes added this to the 0.113 milestone Nov 13, 2024
@guineveresaenger
Copy link
Contributor Author

Update:

It is my suspicion that a Renderer is not being called correctly, causing Walk() to lose place in the document.

  • Running a test like so:
m := goldmark.New(goldmark.WithExtensions(TFRegistryExtension))
	reader := text.NewReader([]byte(tt.input))
	doc := m.Parser().Parse(reader)
	doc.Dump([]byte(tt.input), 0)
	err := m.Renderer().Render(&out, []byte(tt.input), doc)
	fmt.Println(out.String())

shows the parser correctly detects Table nodes, but the out.String() reader shows the truncated output as shown in previous comment.

  • tableRenderer wraps a goldmark-markdown renderer, which we then register the local tableRenderer.render function to.

  • We call the "regular" renderer inside our render func to help with table cell rendering. This is fine and does not transform the source tree.

  • However, the "tablewriter" from https://github.com/olekukonko/tablewriter does not seem to implement Render in a way that allows the writer (?) or the render context(set on Nodes; needs investigation) to keep track of where in the source byte array we are, and the source content, or the reference to it, gets dropped.

table := tablewriter.NewWriter(writer) # this is our writer that we pass in alongside the Table node
table.SetHeader(header)
table.SetBorders(tablewriter.Border{Left: true, Top: false, Right: true, Bottom: false})
table.SetCenterSeparator("|")
table.SetAutoFormatHeaders(false)
table.SetAutoMergeCells(false)
table.SetAutoWrapText(false)
table.SetReflowDuringAutoWrap(false)
table.AppendBulk(rows)
table.Render() # this is not a markdown renderer; it's just a table printer
  • Interestingly, when we call the unextended Render method on extensionast.Table, using a textBlock and the passed-in Writer like so:
case *extensionast.Table:
	tableRow := ast.NewTextBlock()
	err = tableRenderer.Render(writer, source, tableRow)

this does enable subsequent Renderers to track the remainder of the doc again, although of course now we have some unwanted HTML output.

  • finally, annoyingly, it seems impossible to render using tableRenderer.Render outside of Walk().

guineveresaenger added a commit that referenced this issue Nov 19, 2024
This pull request does a couple of things:

- Implement a render function for each subset of Table
- Expand the `tableRenderer` struct to hold state
- initialize a separate Renderer for the table renderer, since the logic
for cell rendering calls back into the outer renderer and trying to use
the same renderer lost track of content.
- add tests to reflect a table in the middle of a document to ensure
text after the table continues to get rendered
- ad an overall doc test that contains a table.

PR using this change: pulumi/pulumi-sdwan#114
Fixes #2466
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Nov 19, 2024
@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #2632 and shipped in release v3.96.0.

guineveresaenger added a commit to pulumi/pulumi-databricks that referenced this issue Dec 2, 2024
This PR automates docsgen for this provider's installation doc.

Part of pulumi/home#3598.

Similar to pulumi/pulumi-newrelic#890, this is
probably blocked on
pulumi/pulumi-terraform-bridge#2466 due to
poor table rendering.

Update: This is awaiting the latest bridge; the table is rendered nicely
now:
<img width="750" alt="Screenshot 2024-11-11 at 10 49 14 AM"
src="https://github.com/user-attachments/assets/becb265c-0e7d-42ee-a89e-b1b49bd3127e">


- **first docsgen**
- **Remove TF-specific headers and clean up top-level Description**
- **Tweak regexp for header text matching**
- **Remove note as well**
- **Remove secrets warning**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Improvements or additions to docs for this repo kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants