Skip to content

Commit

Permalink
Only parse all-lowercase patterns as potential TF properties (#1969)
Browse files Browse the repository at this point in the history 
This pull request asserts that [according to Terraform best
practices](https://www.terraform-best-practices.com/naming) we should
not parse backticked values containing uppercase letters as potential
schema properties.

While it is a convention to use `snake_case` for TF properties, it is a
strong one. The AWS provider [requires
it](https://github.com/hashicorp/terraform-provider-aws/blob/main/docs/naming.md),
and other providers follow this convention strictly as well.

The reason I would like to bring this change is because it allows us to
detect documentation sections that are not actually nested property
types, but rather lists of possible values, as described in #1507 in
pulumi-mongodbatlas. Rather than add an ever-elusive regex for "Possible
values include" et al, we can rely with reasonable confidence on the
fact that TF properties are in general lowercase. See sample comparison
diffs below.

A [comparison diff for
mongodbatlas](https://github.com/pulumi/pulumi-mongodbatlas/compare/missing-settings?expand=1)
shows multiple improvements. There are a few regressions but the main
one is for [a
resource](https://www.pulumi.com/registry/packages/mongodbatlas/api-docs/getthirdpartyintegrations/)
whose upstream doc is so unconventional that our representation of it is
already flawed. Overall, we gain a lot more information than we're
losing - the cost is that in a couple of places we get a nonsensical
appendage of a real TF description. This should be fixed with docs
overrides or by opening an upstream PR.
A [comparison diff for
AWS](https://github.com/pulumi/pulumi-aws/compare/missing-settings?expand=1)
shows even more improvements, with only
`V2modelsBotVersionLocaleSpecification` having a single regression,
because the upstream docs are erroneously using camelCase.
A [comparison diff for
GCP](https://github.com/pulumi/pulumi-gcp/compare/missing-settings?expand=1)
shows only improvements.
A [comparison diff for
Azure](https://github.com/pulumi/pulumi-azure/compare/missing-descriptions?expand=1)
shows only improvements.

**Note** I refactored the tests slightly because I wanted to be able to
run individual tests and leave hints as to what they were testing.

Fixes #1507.
  • Loading branch information
@guineveresaenger
guineveresaenger committed May 13, 2024
2 parents ef5aded + 1d76e74 commit d41e499
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 4 deletions.
6 changes: 3 additions & 3 deletions pkg/tfgen/docs.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,11 +441,11 @@ var (
linkFooterRegexp = regexp.MustCompile(`(?m)^(\[\d+\]):\s(.*)`)

argumentBulletRegexp = regexp.MustCompile(
"^\\s*[*+-]\\s*`([a-zA-z0-9_]*)`\\s*(\\([a-zA-Z]*\\)\\s*)?\\s*[:–-]?\\s*(\\([^\\)]*\\)[-\\s]*)?(.*)",
"^\\s*[*+-]\\s*`([a-z0-9_]*)`\\s*(\\([a-zA-Z]*\\)\\s*)?\\s*[:–-]?\\s*(\\([^\\)]*\\)[-\\s]*)?(.*)",
)

bulletPointRegexStr = "^\\s*[*+-]" // matches any bullet point-like character
attributePathNameRegexStr = "\\s*`([a-zA-z0-9._]*)`" // matches any TF attribute path name
bulletPointRegexStr = "^\\s*[*+-]" // matches any bullet point-like character
attributePathNameRegexStr = "\\s*`([a-z0-9._]*)`" // matches any TF attribute path name

// matches any line starting with a bullet point followed by a TF path or resource name)
attributeBulletRegexp = regexp.MustCompile(
Expand Down
57 changes: 56 additions & 1 deletion pkg/tfgen/docs_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,12 @@ func TestReformatText(t *testing.T) {

func TestArgumentRegex(t *testing.T) {
tests := []struct {
name string
input []string
expected map[docsPath]*argumentDocs
}{
{
name: "Discovers * bullet descriptions",
input: []string{
"* `iam_instance_profile` - (Optional) The IAM Instance Profile to",
"launch the instance with. Specified as the name of the Instance Profile. Ensure your credentials have the correct permission to assign the instance profile according to the [EC2 documentation](http://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use_switch-role-ec2.html#roles-usingrole-ec2instance-permissions), notably `iam:PassRole`.",
Expand All @@ -132,6 +134,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses nested arguments via `object supports the following`",
input: []string{
"* `jwt_configuration` - (Optional) The configuration of a JWT authorizer. Required for the `JWT` authorizer type.",
"Supported only for HTTP APIs.",
Expand All @@ -155,6 +158,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Renders ~> **NOTE:** and continues parsing as expected",
input: []string{
"* `website` - (Optional) A website object (documented below).",
"~> **NOTE:** You cannot use `acceleration_status` in `cn-north-1` or `us-gov-west-1`",
Expand Down Expand Up @@ -187,6 +191,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Displays in-text backticked values as given",
input: []string{
"* `action` - (Optional) The action that CloudFront or AWS WAF takes when a web request matches the conditions in the rule. Not used if `type` is `GROUP`.",
" * `type` - (Required) valid values are: `BLOCK`, `ALLOW`, or `COUNT`",
Expand All @@ -209,6 +214,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Retains second mention of property if named twice",
input: []string{
"* `priority` - (Optional) The priority associated with the rule.",
"",
Expand All @@ -221,6 +227,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Correctly handles markdown sectioning",
input: []string{
"* `allowed_audiences` (Optional) Allowed audience values to consider when validating JWTs issued by Azure Active Directory.",
"* `retention_policy` - (Required) A `retention_policy` block as documented below.",
Expand All @@ -238,6 +245,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Cleans up `TF Optional; Default` parentheses",
input: []string{
"* `launch_template_config` - (Optional) Launch template configuration block. See [Launch Template Configs](#launch-template-configs) below for more details. Conflicts with `launch_specification`. At least one of `launch_specification` or `launch_template_config` is required.",
"* `spot_maintenance_strategies` - (Optional) Nested argument containing maintenance strategies for managing your Spot Instances that are at an elevated risk of being interrupted. Defined below.",
Expand Down Expand Up @@ -282,6 +290,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Doesn't associate unbackticked properties in supports block regexp",
input: []string{
"The following arguments are supported:",
"",
Expand All @@ -304,6 +313,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses `property1`, `property2`, and `property3`'s `subproperty` object supports the following",
input: []string{
"The `grpc_route`, `http_route` and `http2_route`'s `action` object supports the following:",
"",
Expand All @@ -330,6 +340,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses H3 and H4 headers and their bullets as nested properties",
input: []string{
"### certificate_authority_configuration",
"",
Expand All @@ -354,6 +365,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Appends information on newlines to correct nested description",
input: []string{
"* `header` - (Optional) Contains additional header parameters for the connection. Each parameter can contain the following:",
" * `key` - (Required) The key for the parameter.",
Expand All @@ -369,6 +381,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Cleans up tabs",
input: []string{
"* `node_pool_config` (Input only) The configuration for the GKE node pool. ",
" If specified, Dataproc attempts to create a node pool with the specified shape. ",
Expand All @@ -384,6 +397,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses subblock regexp",
input: []string{
"The optional `settings.location_preference` subblock supports:",
"",
Expand Down Expand Up @@ -412,6 +426,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses block regexp",
input: []string{
"The optional `settings.location_preference` subblock supports:",
"",
Expand All @@ -430,6 +445,7 @@ func TestArgumentRegex(t *testing.T) {
},
},
{
name: "Parses sublist regexp",
input: []string{
"The optional `settings.location_preference` subblock supports:",
"",
Expand All @@ -447,11 +463,50 @@ func TestArgumentRegex(t *testing.T) {
"settings.maintenance_window.day": {description: "Day of week (`1-7`), starting on Monday"},
},
},

{
name: "All caps bullet points are not parsed as TF properties",
input: []string{
"* `status` - Status of the AWS PrivateLink connection.",
" Returns one of the following values:",
" * `AVAILABLE` Atlas created the load balancer and the Private Link Service.",
" * `INITIATING` Atlas is creating the network load balancer and VPC endpoint service.",
" * `WAITING_FOR_USER` The Atlas network load balancer and VPC endpoint service are created and ready to receive connection requests. " +
"When you receive this status, create an interface endpoint to continue configuring the AWS PrivateLink connection.",
" * `FAILED` A system failure has occurred.",
" * `DELETING` The Private Link service is being deleted.",
},
expected: map[docsPath]*argumentDocs{
"status": {description: "Status of the AWS PrivateLink connection.\n" +
"Returns one of the following values:\n" +
"* `AVAILABLE` Atlas created the load balancer and the Private Link Service.\n" +
"* `INITIATING` Atlas is creating the network load balancer and VPC endpoint service.\n" +
"* `WAITING_FOR_USER` The Atlas network load balancer and VPC endpoint service are created and ready to receive connection requests. " +
"When you receive this status, create an interface endpoint to continue configuring the AWS PrivateLink connection.\n" +
"* `FAILED` A system failure has occurred.\n*" +
" `DELETING` The Private Link service is being deleted."},
},
},
{
name: "Bullet points in backticks containing any uppercase letters are not parsed as TF properties",
input: []string{
"* `status` - Status of the AWS PrivateLink connection.",
" Returns one of the following values:",
" * `Available` Atlas created the load balancer and the Private Link Service.",
" * `Initiating` Atlas is creating the network load balancer and VPC endpoint service.",
},
expected: map[docsPath]*argumentDocs{
"status": {description: "Status of the AWS PrivateLink connection.\n" +
"Returns one of the following values:\n" +
"* `Available` Atlas created the load balancer and the Private Link Service.\n" +
"* `Initiating` Atlas is creating the network load balancer and VPC endpoint service."},
},
},
}

for _, tt := range tests {
tt := tt
t.Run("", func(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
ret := entityDocs{
Arguments: make(map[docsPath]*argumentDocs),
}
Expand Down

0 comments on commit d41e499

Please sign in to comment.