pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
197 stars 43 forks source link

Docstrings in generated code are dropped if they contain the word "Terraform" instead of getting replaced or fixed #2387

Closed skirsten closed 2 weeks ago

skirsten commented 2 months ago

What happened?

Terraform provider has generated docs like this:

- `region` (String) The region identifier.
  - If the value of this attribute changes, Terraform will destroy and recreate the resource.

this is done by tfplugindocs.

It contains the word "Terraform" which will cause the pulumi-terraform-bridge/pf since v0.35.0 with this change https://github.com/pulumi/pulumi-terraform-bridge/pull/1902/files#diff-0f6072f2900c4ebdcfa03ded811c7ebffcc191c212b3bff6b10dd9a2be07a3ccR1262-R1264 to completely drop the description.

It should instead keep the description with "Terraform" or replace it with "Pulumi". But it should not remove it 😢

Example

The regression is litterally visible in the testprovider as it also affects other docstrings: https://github.com/pulumi/pulumi-terraform-bridge/pull/1902/files#diff-6fa89724a3411d4706b1723409ed805e3e34146194924abb33022031a9662eafL269-R269

And the regression is still there in master (missing description): https://github.com/pulumi/pulumi-terraform-bridge/blob/bdd39dcdb3e39b0673081ab2cd628da37efbbe05/pf/tests/internal/testprovider/cmd/pulumi-resource-tls/schema.json#L268

Output of pulumi about

doesn't matter, bug is present since pf/v0.35.0

Additional context

No response

Contributing

No response

guineveresaenger commented 2 months ago

Hi @skirsten - thank you for filing this issue. We're sorry you're encountering trouble with this.

Unfortunately, this behavior is expected and has been the standard, the reason being that there is no way to translate any "Terraform" string found into "Pulumi" across the board and have all the resulting documentation make sense.

Example: "In Terraform v0.13, the plugin-sdk does X" --> "In Pulumi v0.13, the plugin-sdk does X" ❌ "For users of Terraform Cloud..." --> "For users of Pulumi Cloud..." ❌

There are, of course, plenty of examples where a replacement makes sense, and we do have some defaults for those in the bridge.

As a workaround, custom edit rules can be applied to any provider at schema gen time to avoid this elision.

I'm leaving this issue open because we are discussing changes to our elision policy internally, so again, thank you very much for your perspective here.

skirsten commented 2 months ago

Hi @guineveresaenger,

thanks for the explanation and directing me to the DocRules. I implemented it (see here), but unfortunately its not working.

If instead of the "Terraform" -> "Pulumi" replacement I just replace "a" with "b" it works as expected:

content = bytes.ReplaceAll(content, []byte("a"), []byte("b"))

So I am guessing that this rule is applied after the description is already dropped because it contained "Terraform".

Can you help me here? Is there any way to set the priority for the rules or is there a different way to transform the docs before this?

Edit: I also tried to put the defaults last, without success.

guineveresaenger commented 2 months ago

I've run your changes through the bridge and I see what you're seeing.

I'm pretty baffled here - this should work. Let me look at this some more.

skirsten commented 2 weeks ago

Hi, I just revisited this topic and there does not seem to be any way to implement the rename on the pulumi side before its completely removed.

So to work around this I just added a rule in the terraform provider repo to re-word the line that caused the problem.

https://github.com/genesiscloud/terraform-provider-genesiscloud/blob/eae223e224b7989dae8d9d2ab3b54520d81a3a20/internal/resourceenhancer/markdown.go#L25-L27

A bit hacky, but it works and is simple.