tools/data-api-sdk: Phase 1 and Phase 3
#3784
Conversation
tombuildsstuff
added
refactor
tool/internal-sdk
tombuildsstuff
force-pushed
the
refactor/1-duplicating-models
branch
from
1397493 to
b96600f
Compare
|
|
||
| The majority of the models used in this SDK come from [the `./models` package](./models) - however this package also contains a handful of models for 1:1 compatibility with the API. | ||
|
|
||
| Whilst the SDK contains all the supported endpoints, in the majority of cases the `LoadAllData` method should be sufficient, which can be used like so: |
There was a problem hiding this comment.
Should we call out any other use cases in this readme or is just LoadAllData sufficient?
There was a problem hiding this comment.
LoadAllData is sufficient as an example here - whilst the raw endpoints are in the SDK they're only going to be used in specialised cases so it's fine to leave these undocumented here, since the methods themselves are documented
| const ( | ||
| // BooleanTerraformSchemaObjectDefinitionType specifies that this represents a Boolean value. | ||
| // This is output as a `bool` in Models and as `pluginsdk.TypeBool` within the Terraform Schema. | ||
| BooleanTerraformSchemaObjectDefinitionType TerraformSchemaObjectDefinitionType = "Boolean" |
There was a problem hiding this comment.
Is part of this refactor also consolidating all the constants down? I see these being "copied" in a few places but can't remember if we'll eventually consolidate them down into one package or not?
There was a problem hiding this comment.
In short: mostly. Longer answer:
Today we're combine 2 of the 3 Object Definitions together (which is actually problematic since it means we're not correctly modelling the option fields correctly) - so this PR now intentionally splits the Object Definition 3 ways rather than 2 ways, into:
SDKObjectDefinition- used to represent Golang types (used in the Go SDK).OperationOptionObjectDefinition- used to represent types used in the QueryString/Headers (used in the Go SDK). These are Golang types but only support a subset of values (bool/int/float/string/list/csv/constant).TerraformSchemaObjectDefinition- used to represent the types used in Terraform, which aren't 1:1 with theSDKObjectDefinition(used in the Terraform Generator).
to ensure we're representing those correctly - but all of those models come from the new data-api-sdk package.
Whilst we'll be using those models throughout importer-rest-api-specs (and importer-msgraph-metadata soon) - we'll need to use a different model for persisting those to disk (since not all of the API Definitions are stored as JSON [e.g. the test configurations are HCL] - so we'll ned to transform to/from that) - but we should be able to remove the duplicated types from importer-rest-api-specs, yes.
…odels` This is to enable these to be worked through/cleaned up/refactored
This enables grouping the endpoint specific models within the `v1` directory - whilst the `models` package contains the actual models we're using. This means that downstream applications don't unintentionally import (say) `AvailableServiceSummary`, which contains the URI to obtain the Service from, rather than the Services type containing the full/additional information, which is more useful.
Since the models returned by this endpoint are only for use with the API and aren't useful outside of the SDK, I've opted to house these within the `v1` package - meaning that `models` contains the types intended to be (re)used across the codebase.
This includes aliases for some of the more popular types - and also helpers specifically for the different types of Resource ID Segments (since these are widely used across the codebase).
Notable here is that both `TerraformFieldMappingDefinition` and `TerraformSchemaFieldValidationDefinition` are now Discriminated Types to allow us to type cast as needed.
These are mostly lifted & shifted for now - these continue to want unit tests but I'm splitting that work out for the moment since this changeset has gotten big enough (and this was a pending TODO).
This is a convenience function present to obtain all Services from the Data API in a single call.
…jectDefinition` to get the Golang type for an SDKOperationObjectDefinition
…el wasn't deserialized correctly
tombuildsstuff
force-pushed
the
refactor/1-duplicating-models
branch
from
cdb5577 to
e83d8c5
Compare
| // TODO: determine how to handle these | ||
| // for now treat CSV's as a string |
There was a problem hiding this comment.
Perhaps this would be better left to the generator or the built SDK to handle, since CSV is fraught with implementation differences?
There was a problem hiding this comment.
Yeah indeed, or certainly for the moment anyway.
FWIW this TODO comes from the older SDK and is mostly for investigative purposes, to try and determine if there's anything in the data - which I think is still valid (and is why I've left this in) - but isn't urgent/actionable anytime soon.
| // Complex Types | ||
| models.LocationSDKObjectDefinitionType: "string", | ||
| models.RawFileSDKObjectDefinitionType: "[]byte", | ||
| models.RawObjectSDKObjectDefinitionType: "interface{}", // TODO: should we standardise this on `any`? |
There was a problem hiding this comment.
Would any be a type alias for interface{} in this case? I do think interface{} is fine since it's habitual to look for these as something that needs extra handling consideration.
There was a problem hiding this comment.
As of Go 1.18 any and interface{} are interchangeable (core) types - so this would be mostly a cosmetic change fwiw.
I do think there's some benefit to doing this long term (hence the TODO, since I'd argue that any is clearer than interface{} to folks less familiar with Go), but this is pretty low priority/isn't urgent by any means (since it'd trigger an SDK regeneration when it does happen, so would need to be done once the refactor is completed at any rate).
tools/data-api-sdk/v1/models/terraform_schema_field_validation_possible_values_definition.go
Show resolved
Hide resolved
tools/data-api-sdk/v1/models/terraform_direct_assignment_field_mapping_definition.go
Show resolved
Hide resolved
Co-authored-by: Tom Bamford <[email protected]>
Co-authored-by: Tom Bamford <[email protected]>
…_mapping_definition.go Co-authored-by: Tom Bamford <[email protected]>
…_possible_values_definition.go Co-authored-by: Tom Bamford <[email protected]>
Co-authored-by: Tom Bamford <[email protected]>
Co-authored-by: Tom Bamford <[email protected]>
This PR implements both Phase 1 and Phase 3 of #3754 by building out the updated structs (with improved documentation and more consistent naming) (for Phase 1) - and the associated SDK methods for those (Phase 3).
Whilst we originally intended to do these sequentially, from a testing perspective it made it simpler to combine these stages together - therefore once this PR is merged the next step will be to update the Data API to use these new types (Phase 2).
At a high level the models present within this package are mostly the same as what existed previously, with some updated names (as outlined in #3754) and field names (e.g.
Sdk->SDK) for consistency.As a part of this I've introduced a few directories:
./tools/data-api-sdk/v1<- contains the V1 SDK Client - plus any models which map 1:1 with the API (e.g. 1:1 models)./tools/data-api-sdk/v1/models<- contains the V1 SDK Models, with detailed documentation../tools/data-api-sdk/v1/helpers<- contains helper functions intended to be used across all tools.As a part of working through I've removed some functionality which no longer makes sense/wasn't fully implemented due to shifting requirements:
TerraformSchemaFieldTypeSku(unused/requires reworking)ApiOperationDetails- should be unusedResourceSummary(not widely used)ServiceVersionDetails(not widely used)ManualMappingDefinitionType(currently unused)FieldManualMappingDefinition(currently unused)FieldValidationDetails(no longer required)FieldValidationType(no longer required)SDKConstant- no longer supportsCaseInsensitiveSDKField-IsTypeHintis nowContainsDiscriminatedValueSDKField-TypeHintInis nowFieldNameContainingDiscriminatedValueSDKField-TypeHintValueis nowDiscriminatedValueWhilst these are fairly minor, in passing through it felt worthwhile to standardise this terminology further.
Finally I've opted to use Discriminated Types for both
TerraformFieldMappingDefinitionandTerraformSchemaFieldValidationDefinition- since it allows us (when the refactor is complete) to remove two (now inferable)typefields.