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.

Sign up for GitHub

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

importer-rest-api-specs - Add Schema and Documentation Overrides #3610

Merged
merged 14 commits into from
Jan 16, 2024
Merged

importer-rest-api-specs - Add Schema and Documentation Overrides #3610

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0ed6e19
add SchemaOverrides and DocumentationOverrides attribute to models
stephybun Jan 11, 2024
453c089
add helper to convert from title to snake case
stephybun Jan 11, 2024
492f8d2
prevent error when model object is nil when writing to template
stephybun Jan 11, 2024
961df1a
add SchemaOverrides processor, label rules so that they can be filter…
stephybun Jan 11, 2024
9122a0d
add target_resource_id dependency for chaos studio target
stephybun Jan 11, 2024
39e0bc8
add documentation overrides for resource id fields
stephybun Jan 11, 2024
c9d23ec
add emoji to Note to make it more prominent
stephybun Jan 11, 2024
28dc353
fix test and update readme
stephybun Jan 11, 2024
a402718
remove override properties from data-api models
stephybun Jan 12, 2024
532d0dd
replace document and schema overrides with an override struct
stephybun Jan 12, 2024
3789afa
skip adding mapping if the fields object type is a raw object
stephybun Jan 15, 2024
4650921
remove document, schema overrides
stephybun Jan 15, 2024
d7d677f
add ResourceBuildInfo struct and pass in overrides through this struct
stephybun Jan 15, 2024
34aa2b0
goimports
stephybun Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions tools/data-api-differ/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ Example output:

The following new Static Identifiers were detected from the set of changes (new/updated Resource IDs).

> Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
> ⚠️ Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.

Please review the following list of Static Identifiers:

Expand All @@ -197,7 +197,7 @@ Please review the following list of Static Identifiers:

---

> Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
> ⚠️ Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
```

Example of the Markdown Comment (rendered as Markdown):
Expand All @@ -207,7 +207,7 @@ Example of the Markdown Comment (rendered as Markdown):

The following new Static Identifiers were detected from the set of changes (new/updated Resource IDs).

> Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
> ⚠️ Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.

Please review the following list of Static Identifiers:

Expand All @@ -222,5 +222,5 @@ Please review the following list of Static Identifiers:

---

> Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
> ⚠️ Note: Resource ID segments should **always** be `camelCased` and not `TitleCased`, `lowercased` or `kebab-cased`.
```
4 changes: 2 additions & 2 deletions tools/data-api-differ/internal/views/resource_id_segments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ No new Resource ID Segments containing Static Identifiers were identified in the

The following new Static Identifiers were detected from the set of changes (new/updated Resource IDs).

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.

Please review the following list of Static Identifiers:

Expand All @@ -82,7 +82,7 @@ Please review the following list of Static Identifiers:

---

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
`, strings.Join(lines, "\n"))
output = strings.ReplaceAll(output, "'", "`")
//TODO: add a "see the link for how to fix this" to the comment above when the associated documentation is available
Expand Down
8 changes: 4 additions & 4 deletions tools/data-api-differ/internal/views/resource_id_segments_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestResourceIdSegmentsView_Markdown_WithChanges(t *testing.T) {

The following new Static Identifiers were detected from the set of changes (new/updated Resource IDs).

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.

Please review the following list of Static Identifiers:

Expand All @@ -70,7 +70,7 @@ Please review the following list of Static Identifiers:

---

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
`, "'", "`")
testhelpers.AssertTemplatedCodeMatches(t, expected, *actual)
}
Expand Down Expand Up @@ -148,7 +148,7 @@ func TestResourceIdSegmentsView_Markdown_WithRelevantAndIrrelevantChanges(t *tes

The following new Static Identifiers were detected from the set of changes (new/updated Resource IDs).

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.

Please review the following list of Static Identifiers:

Expand All @@ -162,7 +162,7 @@ Please review the following list of Static Identifiers:

---

> Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
> ⚠️ Note: Resource ID segments should **always** be 'camelCased' and not 'TitleCased', 'lowercased' or 'kebab-cased'.
`, "'", "`")
testhelpers.AssertTemplatedCodeMatches(t, expected, *actual)
}
40 changes: 21 additions & 19 deletions tools/data-api/internal/repositories/terraform_models.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,27 @@ type TerraformDataSourceDetails struct {
}

type TerraformResourceDetails struct {
ApiVersion string
CreateMethod MethodDefinition
DeleteMethod MethodDefinition
Documentation ResourceDocumentationDefinition
DisplayName string
Generate bool
GenerateModel bool
GenerateIdValidation bool
GenerateSchema bool
Label string
Mappings MappingDefinition
ReadMethod MethodDefinition
Resource string
ResourceIdName string
ResourceName string
SchemaModelName string
SchemaModels map[string]TerraformSchemaModelDefinition
Tests TerraformResourceTestsDefinition
UpdateMethod *MethodDefinition
ApiVersion string
CreateMethod MethodDefinition
DeleteMethod MethodDefinition
Documentation ResourceDocumentationDefinition
DocumentationOverrides *map[string]string
stephybun marked this conversation as resolved.
Show resolved Hide resolved
DisplayName string
Generate bool
GenerateModel bool
GenerateIdValidation bool
GenerateSchema bool
Label string
Mappings MappingDefinition
ReadMethod MethodDefinition
Resource string
ResourceIdName string
ResourceName string
SchemaModelName string
SchemaModels map[string]TerraformSchemaModelDefinition
SchemaOverrides *map[string]string
stephybun marked this conversation as resolved.
Show resolved Hide resolved
Tests TerraformResourceTestsDefinition
UpdateMethod *MethodDefinition
}

type MethodDefinition struct {
Expand Down
7 changes: 7 additions & 0 deletions tools/data-api/models/terraform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,10 @@ type TerraformResourceDetails struct {
// for this Resource.
Documentation ResourceDocumentationDefinition `json:"documentation"`

// DocumentationOverrides specifies a mapping of properties that require
// custom descriptions in the Documentation.
DocumentationOverrides *map[string]string `json:"documentationOverrides"`
stephybun marked this conversation as resolved.
Show resolved Hide resolved

// DisplayName is the human-readable/marketing name for this Resource,
// for example `Resource Group` or `Virtual Machine`.
DisplayName string `json:"displayName"`
Expand Down Expand Up @@ -298,6 +302,9 @@ type TerraformResourceDetails struct {
// Terraform Schema Models used in this Resource, including mappings to the SDK Models.
SchemaModels map[string]TerraformSchemaModelDefinition `json:"schemaModels"`

// SchemaOverrides is a mapping of resource properties that require renaming.
SchemaOverrides *map[string]string `json:"schemaOverrides"`
stephybun marked this conversation as resolved.
Show resolved Hide resolved

// Tests defines the Terraform Configurations which should be used to test this Resource.
Tests TerraformResourceTestsDefinition `json:"tests"`

Expand Down
6 changes: 6 additions & 0 deletions tools/generator-terraform/generator/mappings/assignment_model_to_model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ func (m modelToModelAssignmentLine) assignmentForCreateUpdateMapping(mapping res
return nil, fmt.Errorf("couldn't find SDK Field %q in Model %q", mapping.ModelToModel.SdkFieldName, mapping.ModelToModel.SdkModelName)
}
if sdkField.ObjectDefinition.Type != resourcemanager.ReferenceApiObjectDefinitionType {
if sdkField.ObjectDefinition.Type == resourcemanager.RawObjectApiObjectDefinitionType {
return nil, nil
}
stephybun marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("a ModelToModel mapping must be a Reference but got %q", string(sdkField.ObjectDefinition.Type))
}
outputModelName := *sdkField.ObjectDefinition.ReferenceName
Expand Down Expand Up @@ -58,6 +61,9 @@ func (m modelToModelAssignmentLine) assignmentForReadMapping(mapping resourceman
return nil, fmt.Errorf("couldn't find SDK Field %q in Model %q", mapping.ModelToModel.SdkFieldName, mapping.ModelToModel.SdkModelName)
}
if sdkField.ObjectDefinition.Type != resourcemanager.ReferenceApiObjectDefinitionType {
if sdkField.ObjectDefinition.Type == resourcemanager.RawObjectApiObjectDefinitionType {
return nil, nil
}
stephybun marked this conversation as resolved.
Show resolved Hide resolved
return nil, fmt.Errorf("a ModelToModel mapping must be a Reference but got %q", string(sdkField.ObjectDefinition.Type))
}
outputModelName := *sdkField.ObjectDefinition.ReferenceName
Expand Down
8 changes: 6 additions & 2 deletions tools/generator-terraform/generator/mappings/assignments.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,9 @@ func (m *Mappings) SchemaModelToSdkModelAssignmentLine(mappings []resourcemanage
if err != nil {
return nil, fmt.Errorf("building create/update assignment line for assignment type %q (Mapping %q): %+v", string(mapping.Type), mapping.String(), err)
}
lines = append(lines, *assignmentLine)
if assignmentLine != nil {
lines = append(lines, *assignmentLine)
}
stephybun marked this conversation as resolved.
Show resolved Hide resolved
}

out := strings.Join(lines, "\n")
Expand Down Expand Up @@ -176,7 +178,9 @@ func (m *Mappings) SdkModelToSchemaModelAssignmentLine(mappings []resourcemanage
if err != nil {
return nil, fmt.Errorf("building read assignment line for constant assignment type %q: %+v", mapping.Type, err)
}
lines = append(lines, *assignmentLine)
if assignmentLine != nil {
lines = append(lines, *assignmentLine)
}
stephybun marked this conversation as resolved.
Show resolved Hide resolved
}

out := strings.Join(lines, "\n")
Expand Down
22 changes: 22 additions & 0 deletions tools/importer-rest-api-specs/components/helpers/title_case.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package helpers

import (
"unicode"
)

func ConvertFromSnakeToTitleCase(input string) string {
output := make([]rune, 0)

for k, v := range input {
if v == '_' {
continue
}
if k == 0 || (k > 0 && input[k-1] == '_') {
output = append(output, unicode.ToUpper(v))
continue
}
output = append(output, v)
}

return string(output)
}
35 changes: 35 additions & 0 deletions tools/importer-rest-api-specs/components/helpers/title_case_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package helpers

import "testing"

func TestConvertFromSnakeToTitleCase(t *testing.T) {
testData := []struct {
input string
expected string
}{
{
input: "oh_lawd_he_coming",
expected: "OhLawdHeComing",
},
{
input: "dns_name",
expected: "DnsName",
},
{
input: "ThisIsAlreadyTitleCased",
expected: "ThisIsAlreadyTitleCased",
},
{
input: "1_2_3_4",
expected: "1234",
},
}

for _, v := range testData {
t.Logf("[DEBUG] Testing %s", v.input)
actual := ConvertFromSnakeToTitleCase(v.input)
if actual != v.expected {
t.Fatalf("Expected %s but got %s", v.expected, actual)
}
}
}
2 changes: 2 additions & 0 deletions tools/importer-rest-api-specs/components/resources/identification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ func FindCandidates(input services.Resource, resourceDefinitions map[string]defi
Category: resourceMetaData.WebsiteSubcategory,
Description: resourceMetaData.Description,
},
DocumentationOverrides: resourceMetaData.DocumentationOverrides,
SchemaOverrides: resourceMetaData.SchemaOverrides,
Tests: resourcemanager.TerraformResourceTestsDefinition{
TestData: &resourcemanager.TerraformResourceTestDataDefinition{
BasicVariables: resourcemanager.TerraformTestDataVariables{
Expand Down
3 changes: 1 addition & 2 deletions tools/importer-rest-api-specs/components/schema/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package schema

import (
"fmt"

"github.com/hashicorp/go-hclog"
"github.com/hashicorp/pandora/tools/importer-rest-api-specs/components/helpers"
"github.com/hashicorp/pandora/tools/importer-rest-api-specs/components/schema/processors"
Expand Down Expand Up @@ -269,7 +268,7 @@ func (b Builder) schemaFromTopLevelModel(input resourcemanager.TerraformResource
if !ok {
return nil, fmt.Errorf("couldn't find Resource ID named %q", input.ResourceIdName)
}
fieldsWithinResourceId, mappings, err := b.identifyTopLevelFieldsWithinResourceID(resourceId, mappings, input.DisplayName, logger.Named("TopLevelFields ResourceID"))
fieldsWithinResourceId, mappings, err := b.identifyTopLevelFieldsWithinResourceID(resourceId, mappings, &input, logger.Named("TopLevelFields ResourceID"))
Copy link
Contributor

Choose a reason for hiding this comment

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

(I can't comment on the Build method since it's too far up, but assume this comment is there)

Rather than adding the overrides to the resourcemanager.TerraformResourceDetails struct - it'd be worth passing in a secondary struct which contains any additional configuration for this resource (for now, being just the override data) - which allows these overrides to stay scoped to the importer-rest-api-specs tool - can we update this?

Copy link
Member Author

@stephybun stephybun Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After investigating, I don't agree with this.

The overrides that we specify are tied to a specific resource, in a specific version, in a specific service. Pulling them out of the resourcemanager.TerraformResourceDetails struct means we lose the information required to decide on what resource, in which version and service to apply the override.

This update would require us to either nest this secondary struct containing the overrides within a resource, version, service struct or add metadata to the secondary struct containing resource, version, service to be able to determine to which resource/version/service an override applies to, in other words if I pull it out, I don't have to information to know which override information to pass into the Build method. I don't believe the overhead and complexity that this adds is worth the benefit of it remaining scoped to importer-rest-api-specs.

I'm open to revisiting this when we discuss boundaries/separation of concerns for refactoring and making Pandora more manageable.

Copy link
Contributor

Choose a reason for hiding this comment

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

The overrides that we specify are tied to a specific resource, in a specific version, in a specific service. Pulling them out of the resourcemanager.TerraformResourceDetails struct means we lose the information required to decide on what resource, in which version and service to apply the override.

Given the limitation (that I don't expect we'd change) that each Terraform Resource is tied to one specific API Version - therefore we’ve got the Service / API Version for each Resource (available in these types which come from the config files [where the override information will be defined]).

Since we’re iterating over each resource, we’ve got access to the Service, API Version and Resource (Label) in question - so I think it should be possible to pull that out here, where we build up the Terraform Resource, Tests and Example Usage - for example:

diff --git a/tools/importer-rest-api-specs/pipeline/run_importer.go b/tools/importer-rest-api-specs/pipeline/run_importer.go
index 48dd2a8719..5061ce00c1 100644
--- a/tools/importer-rest-api-specs/pipeline/run_importer.go
+++ b/tools/importer-rest-api-specs/pipeline/run_importer.go
@@ -120,21 +120,40 @@ func runImportForService(input RunInput, serviceName string, apiVersionsForServi
                        }
                }

+               type resourceBuildInfo struct {
+                       // TODO: data
+               }
+
+               var buildInfoForResource *resourceBuildInfo
+               if api[0].TerraformServiceDefinition != nil {
+                       for version, versionDetails := range api[0].TerraformServiceDefinition.ApiVersions {
+                               for pkgName, pkgDetails := range versionDetails.Packages {
+                                       resDetails, ok := pkgDetails.Definitions["resource_label"]
+                                       if ok {
+                                               buildInfoForResource = &resourceBuildInfo{
+                                                       Example: resDetails.Overrides.Example, // picking a random field from the override config for this Resource
+                                               }
+                                               break
+                                       }
+                               }
+                       }
+               }
+
                versionLogger.Trace("generating Terraform Details")
                var err error
-               dataForApiVersion, err = task.generateTerraformDetails(dataForApiVersion, versionLogger.Named("TerraformDetails"))
+               dataForApiVersion, err = task.generateTerraformDetails(dataForApiVersion, buildInfoForResource, versionLogger.Named("TerraformDetails"))
                if err != nil {
                        return fmt.Errorf(fmt.Sprintf("generating Terraform Details for Service %q / Version %q: %+v", serviceName, apiVersion, err))
                }

                versionLogger.Trace("generating Terraform Tests")
-               dataForApiVersion, err = task.generateTerraformTests(dataForApiVersion, input.ProviderPrefix, versionLogger.Named("TerraformTests"))
+               dataForApiVersion, err = task.generateTerraformTests(dataForApiVersion, input.ProviderPrefix, buildInfoForResource, versionLogger.Named("TerraformTests"))
                if err != nil {
                        return fmt.Errorf(fmt.Sprintf("generating Terraform Tests for Service %q / Version %q: %+v", serviceName, apiVersion, err))
                }

                versionLogger.Trace("Generating Example Usage from the Terraform Tests")
-               dataForApiVersion, err = task.generateTerraformExampleUsage(dataForApiVersion, input.ProviderPrefix, versionLogger.Named("TerraformExampleUsage"))
+               dataForApiVersion, err = task.generateTerraformExampleUsage(dataForApiVersion, input.ProviderPrefix, buildInfoForResource, versionLogger.Named("TerraformExampleUsage"))
                if err != nil {
                        return fmt.Errorf(fmt.Sprintf("generating Terraform Example Usage for Service %q / Version %q: %+v", serviceName, apiVersion, err))
                }

Whilst the TerraformResourceDetails type is doing a little more than it should today - adding more information into this (and particularly persisting it to disk/the Data API) is going to dramatically increase the complexity down the line.

As such I think we want to make a concerted effort to prevent this information from leaking outside of the importer-rest-api-specs tool for now - meaning that we need to ensure these fields aren’t included in TerraformResourceDetails.

Ultimately this ensures that the importer-rest-api-specs tool is the only thing concerned with the overrides - the TerraformResourceDetails that's written to the API Definitions contains just the overridden result (e.g. the Terraform Resource with any overrides applied) meaning that other tools (Data API, Go SDK Generator etc) are unaware that any overrides have been applied - meaning they're focused on one thing (and doing one thing well).

if err != nil {
return nil, fmt.Errorf("identifying top level fields within Resource ID %q: %+v", resourceId.Id, err)
}
Expand Down
38 changes: 27 additions & 11 deletions tools/importer-rest-api-specs/components/schema/fields_resource_id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/hashicorp/pandora/tools/sdk/resourcemanager"
)

func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.ResourceIdDefinition, mappings *resourcemanager.MappingDefinition, resourceDisplayName string, logger hclog.Logger) (*map[string]resourcemanager.TerraformSchemaFieldDefinition, *resourcemanager.MappingDefinition, error) {
func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.ResourceIdDefinition, mappings *resourcemanager.MappingDefinition, resource *resourcemanager.TerraformResourceDetails, logger hclog.Logger) (*map[string]resourcemanager.TerraformSchemaFieldDefinition, *resourcemanager.MappingDefinition, error) {
out := make(map[string]resourcemanager.TerraformSchemaFieldDefinition, 0)

// first determine whether we are dealing with a nested resource
Expand All @@ -31,7 +31,7 @@ func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.Re
}

if parentResourceFound && parentResourceIdName == "" {
logger.Debug("parent resource detected for %s but was not present in resource's ID definitions", resourceDisplayName)
logger.Debug("parent resource detected for %s but was not present in resource's ID definitions", resource.DisplayName)
}

// if a parent is resource is found and present in the ResourceID mappings then we use this
Expand All @@ -45,22 +45,21 @@ func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.Re
ForceNew: true,
HclName: "name",
Documentation: resourcemanager.TerraformSchemaDocumentationDefinition{
Markdown: descriptionForResourceIDSegment("Name", resourceDisplayName),
Markdown: descriptionForResourceIDSegment("Name", resource.DisplayName, resource.DocumentationOverrides),
},
}

// add the parent resource ID and then the name of the resource
parentResourceSchemaField := helpers.ConvertToSnakeCase(parentResourceIdName)
out[parentResourceIdName] = resourcemanager.TerraformSchemaFieldDefinition{
ObjectDefinition: resourcemanager.TerraformSchemaFieldObjectDefinition{
Type: resourcemanager.TerraformSchemaFieldTypeString,
},
// since this is included in the Resource ID it's implicitly Required/ForceNew
Required: true,
ForceNew: true,
HclName: parentResourceSchemaField,
HclName: helpers.ConvertToSnakeCase(parentResourceIdName),
Documentation: resourcemanager.TerraformSchemaDocumentationDefinition{
Markdown: descriptionForResourceIDSegment(parentResourceIdName, resourceDisplayName),
Markdown: descriptionForResourceIDSegment(parentResourceIdName, resource.DisplayName, resource.DocumentationOverrides),
},
}

Expand Down Expand Up @@ -91,12 +90,17 @@ func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.Re

userConfigurableSegments++
fieldName := strings.Title(v.Name)
hclName := helpers.ConvertToSnakeCase(v.Name)
if i == len(input.Segments)-1 {
// if it's the last one override the name since that'll be the name of this Resource
fieldName = "Name"
hclName = "name"
}
var err error
fieldName, err = updateFieldNameFromSchemaOverrides(fieldName, resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than introducing a new method for this, if we reuse this method we should be able to support this across all fields, rather than just those we're building from within the Resource ID?

Copy link
Member Author

Choose a reason for hiding this comment

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

The current implementation supports overrides for Resource IDs and nested fields, it doesn't for top level fields because this method that you linked isn't called for top level fields and by the looks of it shouldn't be for the moment since it's mainly hardcoding the field details and mappings for properties like location, zones, identity etc.

I disagree about reusing that method on the fields we source from the resource IDs, my reason is explained in point three of the PR description. As a compromise I've removed the schema overrides as a field processor, rewritten this function to applySchemaOverrides to do the overrides. applySchemaOverrides is called both here and by the method linked in the paragraph above, so that nested fields can have overrides applied too.

if err != nil {
return nil, nil, fmt.Errorf("overriding schema property from resource ID: %+v", err)
stephybun marked this conversation as resolved.
Show resolved Hide resolved
}
hclName := helpers.ConvertToSnakeCase(fieldName)

field := resourcemanager.TerraformSchemaFieldDefinition{
ObjectDefinition: resourcemanager.TerraformSchemaFieldObjectDefinition{
Type: resourcemanager.TerraformSchemaFieldTypeString,
Expand All @@ -106,7 +110,7 @@ func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.Re
ForceNew: true,
HclName: hclName,
Documentation: resourcemanager.TerraformSchemaDocumentationDefinition{
Markdown: descriptionForResourceIDSegment(fieldName, resourceDisplayName),
Markdown: descriptionForResourceIDSegment(fieldName, resource.DisplayName, resource.DocumentationOverrides),
},
}
if v.Type == resourcemanager.ResourceGroupSegment {
Expand All @@ -128,7 +132,15 @@ func (b Builder) identifyTopLevelFieldsWithinResourceID(input resourcemanager.Re
return &out, mappings, nil
}

func descriptionForResourceIDSegment(input, resourceDisplayName string) string {
func descriptionForResourceIDSegment(input, resourceDisplayName string, documentationOverrides *map[string]string) string {
if documentationOverrides != nil {
for field, description := range *documentationOverrides {
if strings.EqualFold(input, helpers.ConvertFromSnakeToTitleCase(field)) {
return description
}
}
}

if strings.EqualFold(input, "Name") {
return fmt.Sprintf("Specifies the name of this %s.", resourceDisplayName)
}
Expand All @@ -140,7 +152,11 @@ func descriptionForResourceIDSegment(input, resourceDisplayName string) string {
return fmt.Sprintf("Specifies the %s within which this %s should exist.", wordified, resourceDisplayName)
}

return fmt.Sprintf("Specifies the name of the %s within which this %s should exist.", wordified, resourceDisplayName)
if strings.EqualFold(input, "ResourceGroupName") {
return fmt.Sprintf("Specifies the name of the %s within which this %s should exist.", wordified, resourceDisplayName)
}

return fmt.Sprintf("Specifies the %s of this %s.", wordified, resourceDisplayName)
}

func wordifyParentSegment(input string) string {
Expand Down
Loading
Loading