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

Get extra nested docs from schema map #1650

Merged
merged 8 commits into from
Jan 31, 2024
Merged

Conversation

guineveresaenger
Copy link
Contributor

This pull request aims to improve accuracy for nested property docs descriptions.

It uses the SchemaMap to look up and read Description fields such as this one in the case where trying to obtain documentation form entityDocs does not yield any results.

The original impetus for this change was to address #1045, which is very specific to Config() fields. The Provider configuration docs are special-cased in the bridge and we do not have access to any entityDocs. Instead, we were reading in the TF schema's Description() field into rawdocs when generating a top-level config variable. This did not persist into any nested fields on Configuration variables, hence this fix. But it turns out that we can catch quite a few more missing descriptions if we extend implementation across all nested fields (see stats below).

As a side note - the Terraform Plugin Framework implements Description() in a way that respects and prioritizes descriptions that come from Markdown docs; for sdkv2 resources we still need to use, and prefer, our parsed-from-markdown entityDocs descriptions. It would be interesting to see if we get improved docs for TFPF resources by not reading descriptions from entityDocs.
It was also necessary to return an empty string from TFPF typeSchema so as not to cause a panic during schemagen on TFPF using providers. Open to improvements/suggestions here!

This PR aims to be fully additive - we are not changing the way in which docs are generated; we only fill in missing docs.
Nevertheless, this PR does also seem to improve the correctness of nested docs where some description fields have been matched to the wrong input field.

Please see a new Docker schema generated against these changes
Quick stats:

pulumi/pulumi-docker🦉 git diff master --stat -- provider/cmd
 provider/cmd/pulumi-resource-docker/schema.json | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------
 1 file changed, 68 insertions(+), 43 deletions(-)

Here is an AWS schema - you will want to pull that branch and look at the git diff to master locally.
Quick stats:

pulumi/pulumi-aws🦉 git diff master --stat -- provider/cmd 
 provider/cmd/pulumi-resource-aws/schema.json | 1810 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 1206 insertions(+), 604 deletions(-)

This PR fixes #1045.

@iwahbe
Copy link
Member

iwahbe commented Jan 26, 2024

I can see the docker diff in GH: pulumi/pulumi-docker@master...nested-configs-sample-schema.

I believe that the corrections come from (downstream from the change):

func (g *schemaGenerator) genProperty(prop *variable) pschema.PropertySpec {
description := ""
if prop.doc != "" && prop.doc != elidedDocComment {
description = g.genDocComment(prop.doc)
} else if prop.rawdoc != "" {
description = g.genRawDocComment(prop.rawdoc)
}

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big improvement, and I'm looking forward to getting this merged. I think we need to address one additional category of errors before merging.

pkg/tfgen/generate.go Show resolved Hide resolved
@@ -98,8 +98,8 @@ func (*typeSchema) DefaultValue() (interface{}, error) {
return nil, bridge.ErrSchemaDefaultValue
}

func (*typeSchema) Description() string {
panic("Description() should not be called during schema generation")
func (s *typeSchema) Description() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "s" not needed.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minuscule formatting nits. Otherwise LGTM!

Comment on lines 29 to 40
member := func(mod string, mem string) tokens.ModuleMember {
return tokens.ModuleMember("cloudflare" + ":" + mod + ":" + mem)
}

typ := func(mod string, typ string) tokens.Type {
return tokens.Type(member(mod, typ))
}

resource := func(mod string, res string) tokens.Type {
fn := string(unicode.ToLower(rune(res[0]))) + res[1:]
return typ(mod+"/"+fn, res)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: There is a tfbridge.MakeResource(pkg, mod, res) function which does this already.

},
"action_parameters": {
Type: schema.TypeList,
// MaxItems: 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// MaxItems: 1,

@@ -143,6 +154,8 @@ func assertHasSimpleObjectAttributes(t *testing.T, r shim.Resource) {
assert.True(t, r.Schema().Get("c").Computed(), "c is computed")
assert.True(t, r.Schema().Get("r").Required(), "r is required")
assert.True(t, r.Schema().Get("co").Computed() && r.Schema().Get("co").Optional(), "co is computed and optional")
assert.Equal(t, r.Schema().Get("desc").Description(), "I am a description")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@guineveresaenger guineveresaenger merged commit be7f79a into master Jan 31, 2024
7 checks passed
@guineveresaenger guineveresaenger deleted the guin/docs-nested-config branch January 31, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: tfgen does not include descriptions for nested config fields
3 participants