chore: Generate ID and ObjectType Show Object Methods #3292
Conversation
…nowflake into sdk-helper-methods
…nowflake into sdk-helper-methods
…nowflake into sdk-helper-methods
…nowflake into sdk-helper-methods
…nowflake into sdk-helper-methods
…nowflake into sdk-helper-methods
sfc-gh-fbudzynski
requested review from
sfc-gh-jmichalak and
sfc-gh-asawicki
|
Integration tests failure for 38b8a3e322265c1b53426416e494dade4b2d54b5 |
|
Integration tests failure for 9f29c422c3fb2175fcdcd555a6b47c010a02a993 |
| } | ||
|
|
||
| // HelperMethodID adds a helper method "ID()" to the interface file that returns the ObjectIdentifier of the object | ||
| func (i *Interface) HelperMethodID() *Interface { |
There was a problem hiding this comment.
renamed to ResourceIDHelperMethod because in ShowOperation we provide the plainStruct representing resource as resourceRepresentation
It could be named to something more suitable tho
…owflake into sdk-helper-methods
|
Integration tests failure for 89876035c5ca299ff39b58ac321972b4651ad1b2 |
|
Integration tests failure for 05914c304c19eb9003b8e365d06f13e15e5728e5 |
pkg/sdk/api_integrations_def.go
Outdated
| @@ -150,6 +150,7 @@ var ApiIntegrationsDef = g.NewInterface( | |||
| Show(). | |||
| SQL("API INTEGRATIONS"). | |||
| OptionalLike(), | |||
| g.ResourceIDHelperMethod, | |||
There was a problem hiding this comment.
Resource is bad, because it's coupled with TF resource, and SDK should not be coupled with TF. My proposals are g.ShowObjectIdMethod and g.ObjectIdMethod. I like the former more.
There was a problem hiding this comment.
g.ShowObjectIdMethod and g.ShowObjectTypeMethod seems fine to me as well
pkg/sdk/secrets_gen.go
Outdated
| func (s *Secret) ID() SchemaObjectIdentifier { | ||
| return NewSchemaObjectIdentifier(s.DatabaseName, s.SchemaName, s.Name) | ||
| func (v *Secret) ID() SchemaObjectIdentifier { | ||
| return NewSchemaObjectIdentifier(v.Name, v.DatabaseName, v.SchemaName) |
pkg/sdk/secrets_def.go
Outdated
| @@ -241,6 +241,8 @@ var SecretsDef = g.NewInterface( | |||
| SQL("SECRETS"). | |||
| OptionalLike(). | |||
| OptionalExtendedIn(), | |||
| g.ResourceIDHelperMethod, | |||
| g.ResourceObjectTypeHelperMethod, | |||
There was a problem hiding this comment.
Similarly to the comment above: g.ShowObjectTypeMethod?
There was a problem hiding this comment.
seems good
| @@ -21,9 +21,18 @@ func GenerateInterface(writer io.Writer, def *Interface) { | |||
| if o.OptsField != nil { | |||
| generateOptionsStruct(writer, o) | |||
| } | |||
| if o.ResourceHelperMethods != nil { | |||
There was a problem hiding this comment.
Nit: this check is not necessary - when a slice is nil, for ... range does not iterate.
| op.ObjectInterface = i | ||
| op.withFiltering(append([]ShowByIDFilteringKind{filter}, filtering...)...) | ||
| op := newNoSqlOperation(string(OperationKindShowByID)). | ||
| withObjectInterface(i). |
| } | ||
| } | ||
|
|
||
| var requiredFieldsForIDMethodMapping map[objectIdentifier][]string = map[objectIdentifier][]string{ |
There was a problem hiding this comment.
var idTypeParts = map[objectIdentifier][]string{ , wdyt?
There was a problem hiding this comment.
seems all right
| SchemaObjectIdentifierWithArguments objectIdentifier = "SchemaObjectIdentifierWithArguments" | ||
| ) | ||
|
|
||
| func identifierStringToObjectIdentifier(s string) objectIdentifier { |
There was a problem hiding this comment.
Nit: pls add basic unit tests.
| objectIdentifier := identifierStringToObjectIdentifier(identifierString) | ||
| requiredFields, ok := requiredFieldsForIDMethodMapping[objectIdentifier] | ||
| if !ok { | ||
| log.Printf("WARNING: No required fields mapping defined for identifier %s", objectIdentifier) |
There was a problem hiding this comment.
Nit: we us [WARN] prefix for such logs.
| SchemaObjectIdentifier: {"Name", "DatabaseName", "SchemaName"}, | ||
| } | ||
|
|
||
| func hasRequiredFieldsForIDMethod(structName string, helperStructs []*Field, requiredFields ...string) bool { |
There was a problem hiding this comment.
Nit: we should receive requiredFields ...objectIdentifier and convert to string inside.
| return newResourceHelperMethod("ObjectType", structName, "ObjectType"+structName, "ObjectType") | ||
| } | ||
|
|
||
| func (s *Operation) withResourceHelperMethods(structName string, helperMethods ...ResourceHelperMethodKind) *Operation { |
There was a problem hiding this comment.
Nit: in general, it's easier to read a file when upstream functions are below functions that call them. So, this function should be moved higher than newResourceIDHelperMethod and newResourceObjectTypeHelperMethod.
|
Integration tests failure for 268e2d8590a4659e699c66f75ca75841698daac8 |
|
Integration tests failure for 9e757be8daa86a0ea39820a52d7e183208c76a74 |
sfc-gh-fbudzynski
changed the title
|
Integration tests failure for c60dec58a0f43da3e9bf4126eb358889ca0c101e |
|
Integration tests failure for a9d50cb6fd168a1818d4e2038ff268f7edc1c413 |
|
Integration tests failure for 57ebbc34fb17754d7e0e1ab7df9a62ad02ea4dfe |
|
Integration tests failure for 560cc8b84fbd42697bf5d898308ea91743b01918 |
pkg/sdk/poc/generator/operation.go
Outdated
| @@ -40,6 +40,8 @@ type Operation struct { | |||
| DescribeMapping *Mapping | |||
| // ShowByIDFiltering defines a kind of filterings performed in ShowByID operation | |||
| ShowByIDFiltering []ShowByIDFiltering | |||
| // ShowObjectMethods contains helper methods for the Interface file (i.e. ID(), ObjectType()) | |||
| ShowObjectMethods []*ShowObjectMethod | |||
There was a problem hiding this comment.
I think this should not be a configurable perk but rather a permanent feature for each compatible object. Because of that let's:
- remove this
ShowObjectMethods(and all added modifications from this file) - for each generated show operation let's add a conditional generation (if show output contains all needed fields, then generate ID() func, and always generate objectType func)
| @@ -0,0 +1,6 @@ | |||
| {{- /*gotype: github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/sdk/poc/generator.ResourceHelperMethod*/ -}} | |||
There was a problem hiding this comment.
let's have dedicated templates for both functions and let's move more logic there (e.g. for ObjectType method we should require only structName, for identifier we should require id type and args and handle it inside the template)
…owflake into sdk-helper-methods
|
Integration tests failure for 5f5e26326b475687677b11f7a579267e8b173113 |
|
Integration tests failure for cae24f6f5b8bbf68767e342ef07bbee9df13c0ce |
Changes
Examples
Generated examples for:
SchemaObjectIdentifier
AccountObjectIdentifier