Skip to content

Commit

Permalink
Get extra nested docs from schema map (#1650)
Browse files Browse the repository at this point in the history
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](https://github.com/kreuzwerker/terraform-provider-docker/blob/master/internal/provider/provider.go#L111)
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](https://github.com/pulumi/pulumi-docker/blob/nested-configs-sample-schema/provider/cmd/pulumi-resource-docker/schema.json)
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](https://github.com/pulumi/pulumi-aws/blob/testing-docsgen/provider/cmd/pulumi-resource-aws/schema.json) - 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.
  • Loading branch information
guineveresaenger authored Jan 31, 2024
2 parents e730ea4 + 244cc22 commit be7f79a
Show file tree
Hide file tree
Showing 13 changed files with 441 additions and 94 deletions.
2 changes: 1 addition & 1 deletion internal/testprovider/schema_mini_cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)

// Subset of pulumi-random provider.
// Subset of pulumi-cloudflare provider, specifically to test nested descriptions cleanup
func ProviderMiniCloudflare() *schema.Provider {
resourceRuleset := func() *schema.Resource {
return &schema.Resource{
Expand Down
92 changes: 92 additions & 0 deletions internal/testprovider/schema_nested_descriptions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Copyright 2016-2022, Pulumi Corporation.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package testprovider

import (
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
)

// Subset of pulumi-cloudflare provider.
func ProviderNestedDescriptions() *schema.Provider {
resourceRuleset := func() *schema.Resource {
return &schema.Resource{
Description: "Deploy a ruleset for cloudflare",
Schema: resourceNestedDescriptionsSchema(),
}
}

return &schema.Provider{
Schema: map[string]*schema.Schema{},
ResourcesMap: map[string]*schema.Resource{
"cloudflare_ruleset": resourceRuleset(),
},
}
}

// An aggressively cut down version of cloudflare_ruleset.
func resourceNestedDescriptionsSchema() map[string]*schema.Schema {
return map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Required: true,
ForceNew: true,
Description: "Name of the ruleset.",
},
"description": {
Type: schema.TypeString,
Optional: true,
Description: "Brief summary of the ruleset and its intended use.",
},
"rules": {
Type: schema.TypeList,
Optional: true,
Description: "List of rules to apply to the ruleset.",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"id": {
Type: schema.TypeString,
Computed: true,
Description: "Unique rule identifier.",
},
"version": {
Type: schema.TypeString,
Computed: true,
Description: "Version of the ruleset to deploy.",
},
"action_parameters": {
Type: schema.TypeList,
Optional: true,
Description: "List of parameters that configure the behavior of the ruleset rule action.",
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"id": {
Type: schema.TypeString,
Optional: true,
Description: "Identifier of the action parameter to modify. " +
"When Terraform is mentioned here, the description should be dropped.",
},
"translateField": {
Type: schema.TypeString,
Optional: true,
Description: "When cloudflare_ruleset is mentioned, it should be translated.",
},
},
},
},
},
},
},
}
}
18 changes: 15 additions & 3 deletions pf/internal/schemashim/object_type_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,21 @@ func TestObjectAttribute(t *testing.T) {
assert.Equal(t, shim.TypeString, s.Type())
}

func TestTypeSchemaDescriptionIsEmpty(t *testing.T) {
shimmedType := &typeSchema{
t: types.StringType,
nested: nil,
}
assert.Equal(t, shimmedType.Description(), "")
}

func TestSingleNestedBlock(t *testing.T) {
b := schema.SingleNestedBlock{
Attributes: simpleObjectAttributes(),
}
shimmed := &blockSchema{"key", pfutils.FromResourceBlock(b)}
assertIsObjectType(t, shimmed)
assert.Equal(t, "obj[c=str,co=str,o=str,r=str]", schemaLogicalType(shimmed).String())
assert.Equal(t, "obj[c=str,co=str,desc=str,o=str,r=str]", schemaLogicalType(shimmed).String())
r, ok := shimmed.Elem().(shim.Resource)
require.True(t, ok, "Single-nested TF blocks should be represented as Elem() shim.Resource")
assertHasSimpleObjectAttributes(t, r)
Expand All @@ -61,7 +69,7 @@ func TestListNestedBlock(t *testing.T) {
},
}
shimmed := &blockSchema{"key", pfutils.FromResourceBlock(b)}
assert.Equal(t, "list[obj[c=str,co=str,o=str,r=str]]", schemaLogicalType(shimmed).String())
assert.Equal(t, "list[obj[c=str,co=str,desc=str,o=str,r=str]]", schemaLogicalType(shimmed).String())
r, ok := shimmed.Elem().(shim.Resource)
require.True(t, ok, "List-nested TF blocks should be represented as Elem() shim.Resource")
assertHasSimpleObjectAttributes(t, r)
Expand All @@ -74,7 +82,7 @@ func TestSetNestedBlock(t *testing.T) {
},
}
shimmed := &blockSchema{"key", pfutils.FromResourceBlock(b)}
assert.Equal(t, "set[obj[c=str,co=str,o=str,r=str]]", schemaLogicalType(shimmed).String())
assert.Equal(t, "set[obj[c=str,co=str,desc=str,o=str,r=str]]", schemaLogicalType(shimmed).String())
r, ok := shimmed.Elem().(shim.Resource)
require.True(t, ok, "Set-nested TF blocks should be represented as Elem() shim.Resource")
assertHasSimpleObjectAttributes(t, r)
Expand Down Expand Up @@ -135,6 +143,9 @@ func simpleObjectAttributes() map[string]schema.Attribute {
Computed: true,
Optional: true,
},
"desc": schema.StringAttribute{
Description: "I am a description",
},
}
}

Expand All @@ -143,6 +154,7 @@ 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")
}

func assertIsObjectType(t *testing.T, shimmed shim.Schema) {
Expand Down
2 changes: 1 addition & 1 deletion pf/internal/schemashim/type_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (*typeSchema) DefaultValue() (interface{}, error) {
}

func (*typeSchema) Description() string {
panic("Description() should not be called during schema generation")
return ""
}

func (*typeSchema) StateFunc() shim.SchemaStateFunc {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,25 @@
"testbridge:index/TestnestRule:TestnestRule": {
"properties": {
"actionParameters": {
"$ref": "#/types/testbridge:index/TestnestRuleActionParameters:TestnestRuleActionParameters"
"$ref": "#/types/testbridge:index/TestnestRuleActionParameters:TestnestRuleActionParameters",
"description": "List of parameters that configure the behavior of the ruleset rule action.\n"
},
"protocol": {
"type": "string"
"type": "string",
"description": "network protocol\n"
}
},
"type": "object"
},
"testbridge:index/TestnestRuleActionParameters:TestnestRuleActionParameters": {
"properties": {
"automaticHttpsRewrites": {
"type": "boolean"
"type": "boolean",
"description": "Turn on or off Cloudflare Automatic HTTPS rewrites.\n"
},
"bic": {
"type": "boolean"
"type": "boolean",
"description": "Inspect the visitor's browser for headers commonly associated with spammers and certain bots.\n"
},
"phases": {
"$ref": "#/types/testbridge:index/TestnestRuleActionParametersPhases:TestnestRuleActionParametersPhases"
Expand All @@ -70,27 +74,32 @@
"testbridge:index/TestnestRuleActionParametersPhases:TestnestRuleActionParametersPhases": {
"properties": {
"p1": {
"type": "boolean"
"type": "boolean",
"description": "The first phase.\n"
},
"p2": {
"type": "boolean"
"type": "boolean",
"description": "The second phase.\n"
}
},
"type": "object"
},
"testbridge:index/TestnestService:TestnestService": {
"properties": {
"intport": {
"type": "integer"
"type": "integer",
"description": "Port application listens on internally\n"
},
"ports": {
"type": "array",
"items": {
"$ref": "#/types/testbridge:index/TestnestServicePort:TestnestServicePort"
}
},
"description": "External ports and handlers\n"
},
"protocol": {
"type": "string"
"type": "string",
"description": "network protocol\n"
}
},
"type": "object",
Expand All @@ -106,10 +115,12 @@
"type": "array",
"items": {
"type": "string"
}
},
"description": "How the edge should process requests\n"
},
"port": {
"type": "integer"
"type": "integer",
"description": "External port\n"
}
},
"type": "object",
Expand All @@ -120,16 +131,19 @@
"testbridge:index/TestnestattrService:TestnestattrService": {
"properties": {
"intport": {
"type": "integer"
"type": "integer",
"description": "Port application listens on internally\n"
},
"ports": {
"type": "array",
"items": {
"$ref": "#/types/testbridge:index/TestnestattrServicePort:TestnestattrServicePort"
}
},
"description": "External ports and handlers\n"
},
"protocol": {
"type": "string"
"type": "string",
"description": "network protocol\n"
}
},
"type": "object",
Expand All @@ -145,10 +159,12 @@
"type": "array",
"items": {
"type": "string"
}
},
"description": "How the edge should process requests\n"
},
"port": {
"type": "integer"
"type": "integer",
"description": "External port\n"
}
},
"type": "object",
Expand All @@ -159,16 +175,19 @@
"testbridge:index/TestresService:TestresService": {
"properties": {
"intport": {
"type": "integer"
"type": "integer",
"description": "Port application listens on internally\n"
},
"ports": {
"type": "array",
"items": {
"$ref": "#/types/testbridge:index/TestresServicePort:TestresServicePort"
}
},
"description": "External ports and handlers\n"
},
"protocol": {
"type": "string"
"type": "string",
"description": "network protocol\n"
}
},
"type": "object",
Expand All @@ -184,10 +203,12 @@
"type": "array",
"items": {
"type": "string"
}
},
"description": "How the edge should process requests\n"
},
"port": {
"type": "integer"
"type": "integer",
"description": "External port\n"
}
},
"type": "object",
Expand Down
Loading

0 comments on commit be7f79a

Please sign in to comment.