importer-rest-api-specs - Add Schema and Documentation Overrides #3610
Conversation
…ed and apply specific rule to resource ID sources schema fields
Breaking ChangesNo Breaking Changes were found 👍 |
Summary of ChangesNo Breaking or Non-Breaking Changes were found 👍 |
New Resource ID Segments containing Static IdentifiersNo new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙. |
stephybun
force-pushed
the
f/add-schema-docs-overrides
branch
from
b01953b to
28dc353
Compare
Breaking ChangesNo Breaking Changes were found 👍 |
Summary of ChangesNo Breaking or Non-Breaking Changes were found 👍 |
New Resource ID Segments containing Static IdentifiersNo new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙. |
There was a problem hiding this comment.
hey @stephybun
Thanks for this PR - I've taken a look through and left some comments inline, but on the whole this is looking pretty good - in general the comments fall into 1 of 3 categories:
Firstly, since we’re likely to introduce additional overrides in the future, rather than having a map for name overrides and another one for description overrides - we should combine these into an override object from the start (having all fields optional - and assuming that at least one will be specified for each override).
Secondly, since the importer-rest-api-specs tool is (currently) the tool which identifies and then generates resources - rather than adding the override information to the API Definitions/Data API models - can we keep this information constrained to the importer-rest-api-specs tool, rather than persisting it in the API Definitions/Data API Models?
Finally, I think it’d be worth introducing a struct that contains the information we need when building resources (to start with, that’d likely be just the overrides, but it’s worth making that a struct since there’s potentially other things that’d be useful there in the future) - and threading that through Builder struct. This’d allow us to both keep a clearer separation of concerns (since it would be independent from the models used by the Data API/API Definitions) but would also make it easier to thread any additional information through as needed down the line.
That said, on the whole this is looking pretty good - so if we can fix up the comments then this is otherwise headed in the right direction 👍
Thanks!
tools/importer-rest-api-specs/components/schema/fields_resource_id.go
Outdated
Show resolved
Hide resolved
| // fieldNameSchemaOverrideRename{}, | ||
| //} | ||
|
|
||
| var NamingRules = map[string]FieldNameProcessor{ | ||
| // Exists should be first rule in the list since that checks whether the field even exists in the model |
There was a problem hiding this comment.
Maps are unordered, so the ordering is no longer going to be guaranteed here. Instead of adding the name as the key for the map, we can add this to the FieldNameProcessor interface so that each Processor defines a Name, which we can rely on in the same fashion - but that would allow this to remain ordered?
| "github.com/hashicorp/pandora/tools/importer-rest-api-specs/components/helpers" | ||
| "strings" |
There was a problem hiding this comment.
(nit) but goimports
| "github.com/hashicorp/pandora/tools/importer-rest-api-specs/components/helpers" | |
| "strings" | |
| "strings" | |
| "github.com/hashicorp/pandora/tools/importer-rest-api-specs/components/helpers" |
tools/generator-terraform/generator/mappings/assignment_model_to_model.go
Outdated
Show resolved
Hide resolved
tools/generator-terraform/generator/mappings/assignment_model_to_model.go
Outdated
Show resolved
Hide resolved
| } | ||
| var err error | ||
| fieldName, err = updateFieldNameFromSchemaOverrides(fieldName, resource) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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")) | |||
There was a problem hiding this comment.
(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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.TerraformResourceDetailsstruct 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).
Breaking ChangesNo Breaking Changes were found 👍 |
Summary of ChangesNo Breaking or Non-Breaking Changes were found 👍 |
New Resource ID Segments containing Static IdentifiersNo new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙. |
Breaking ChangesNo Breaking Changes were found 👍 |
Summary of ChangesNo Breaking or Non-Breaking Changes were found 👍 |
New Resource ID Segments containing Static IdentifiersNo new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙. |
Breaking ChangesNo Breaking Changes were found 👍 |
Summary of ChangesNo Breaking or Non-Breaking Changes were found 👍 |
New Resource ID Segments containing Static IdentifiersNo new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙. |
This PR adds functionality that will allow us to specify schema property renames and documentation overrides in the resource definition for Pandora.