Add reflect lib for auto generation #1205

wsquan171 · 2024-05-07T22:09:26Z

This PR is a continuation of previous work by @annakhm on test-reflect branch on the following aspects.

Functional

Support conversion of primitive schema types: int, bool, string
Support conversion of nested schema types: list, set
Nested struct in sdk model should be represented as list with at most 1 element, with metadata SchemaType set to struct
List or set of primitive data types (int, bool, string) should use *ExtendedSchema as Schema.Elem
List or set of struct data types should use *ExtendedResource as Schema.Elem, and set Metadata.ReflectType to the type of element type in the slice, rather than a slice type.
UT under metadata/metaddata_test.go can be referenced as example use cases.

Non-Functional

Refactored reflect code into own package for adding isolated UT
Moved version check logic to utils package, so both nsxt and metadata package can share the code
Added Github action for UT

TBD in a future PR

Support of polymorphous struct or list of structs
Support of Schema.TypeMap

wsquan171 · 2024-05-07T22:27:57Z

/test-all

wsquan171 · 2024-05-08T20:04:15Z

/test-all

nsxt/resource_nsxt_policy_mac_discovery_profile.go

nsxt/metadata/metadata.go

nsxt/resource_nsxt_policy_mac_discovery_profile.go

annakhm · 2024-05-10T00:05:13Z

nsxt/resource_nsxt_policy_mac_discovery_profile.go

@@ -98,28 +163,20 @@ func resourceNsxtPolicyMacDiscoveryProfileCreate(d *schema.ResourceData, m inter
 		return err
 	}

+	// TODO - consider including standard object attributes in the schema
+	tags := getPolicyTagsFromSchema(d)


Do we want to include tags, display_name and description in the extended schema?

It's not hard to improve GetExtendedSchema and make it return common extended schema for some well-known policy attrs, but it'll need some refactor on the test side of stuff to make everything e2e working. Can we tackle this after the polymorphic struct is addressed?

annakhm · 2024-05-13T20:54:48Z

Lets make sure we cover the following use cases, either in unit tests or with a resource:

list of native types (int, string, bool)
set of native types
list of structs
set of structs
pointer to struct in the model (list of with maxItems = 1)
double-nested struct or list of structs
list of polymorphic structs (next PR)
set of polymorphic structs (next PR)
pointer to polymorphic struct (next PR)

wsquan171 · 2024-05-21T21:10:17Z

/test-all

annakhm · 2024-05-22T21:44:19Z

nsxt/metadata/metadata.go

+					} else {
+						sliceElem.Index(i).Set(reflect.ValueOf(v))
+					}
+					log.Printf("[INFO] appending %v to %s", v, key)


I think those printouts should be TRACE level, and we might want to preface them with some keyword so that the context of conversion is clear

moved most to TRACE, except for the ones logged before returning an error.

annakhm · 2024-05-22T21:48:44Z

nsxt/metadata/metadata.go

+			}
+
+			if len(itemList) == 0 {
+				continue


I wonder if here we might run into a problem of assigning nil value to array instead of assigning empty array?
We need to be able to remove a list of values from a struct, and in some cases nil value would be ignored by the Patch call.
We can leave this as is for now, and consider adding another metadata value that would control this behavior when the need arises.

Nice catch. I actually think there is no harm in passing an empty slice if tf schema is empty. For create this is a no op, and for update this is an explict erasure. We can revisit this part if other handling for erasing is needed for some NSX resources.

salv-orlando

Test module looks good enough. Only a few questions inline.

nsxt/metadata/metadata.go

salv-orlando · 2024-05-28T06:05:57Z

nsxt/metadata/metadata.go

+		if len(parent) > 0 {
+			log.Printf("[INFO] parent %s key %s", parent, key)
+		}
+		if elem.FieldByName(item.Metadata.SdkFieldName).IsNil() {


According to golang doc, FieldByName will return 0 if the field is not found.
If the method is invoked with the right parameters this should never happen, but in theory it should be possible to have a condition where item.Metadata.SdkFieldName is not defined in elem. If that's correct, adding an additional check would not hurt.

Agree with Salvatore, we should ease troubleshooting for bad metadata arguments that will inevitably occur

added check for elem.FieldByName(item.Metadata.SdkFieldName).IsValid() and returns error if the field is not found

salv-orlando · 2024-05-28T06:19:00Z

nsxt/metadata/metadata.go

+
+		log.Printf("[INFO] inspecting key %s with type %s", key, item.Metadata.SchemaType)
+		if len(parent) > 0 {
+			log.Printf("[INFO] parent %s key %s", parent, key)


If there is value in this log line, we should consider printing something also if len(parent)<=0 since we are also emitting key in the log statement.

added the info of parent as part of context prefix as Anna suggested.

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

In terraform, optional fields all comes with default value. No need to leave them as empty pointer in the reflected struct. Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

This change improves logging with: - include filename and line for log print in metadata pkg - switch to TRACE - add context of the struct being handled Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

If tf schema is updated to empty, the sdk obj should be updated to empty slice instead of nil slice, so that it will be in the PATCH call to remove and clear the list on NSX Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

This change improves error checking and reporting in the reflect lib. Now an error will be returned on both convert directions if either: - The name of a field is not found in the passed in struct - reflect called panic for whatever operation failed Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

wsquan171 · 2024-05-30T18:10:02Z

/test-all

vmwclabot added the dco-required label May 7, 2024

wsquan171 requested review from ksamoray, annakhm and salv-orlando May 7, 2024 22:13

wsquan171 force-pushed the test-reflect branch 2 times, most recently from d372589 to 2fa3325 Compare May 7, 2024 22:26

vmwclabot removed the dco-required label May 7, 2024

vmware deleted a comment from vmwclabot May 7, 2024

annakhm reviewed May 9, 2024

View reviewed changes

nsxt/resource_nsxt_policy_mac_discovery_profile.go Outdated Show resolved Hide resolved

annakhm reviewed May 9, 2024

View reviewed changes

nsxt/metadata/metadata.go Show resolved Hide resolved

annakhm reviewed May 9, 2024

View reviewed changes

nsxt/metadata/metadata.go Outdated Show resolved Hide resolved

annakhm reviewed May 9, 2024

View reviewed changes

nsxt/resource_nsxt_policy_mac_discovery_profile.go Outdated Show resolved Hide resolved

annakhm reviewed May 10, 2024

View reviewed changes

wsquan171 force-pushed the test-reflect branch 2 times, most recently from 0dabad9 to fe820f1 Compare May 21, 2024 19:14

annakhm reviewed May 22, 2024

View reviewed changes

salv-orlando reviewed May 28, 2024

View reviewed changes

annakhm and others added 7 commits May 29, 2024 10:13

PoC using reflect for resource implementation

5a76953

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

Support nested structures

97d76c7

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

Implement extended schema

7fd2275

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

Refactor reflect and some util code to own pkg

d7b497b

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Add UT for reflect lib

0603047

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Handle unset optional items in schema

a923af5

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Add support for list and set

8d7efec

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

wsquan171 added 6 commits May 29, 2024 10:23

Add github workflow for UT

c7882d3

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Remove empty value check for basic schema types

1379ad6

In terraform, optional fields all comes with default value. No need to leave them as empty pointer in the reflected struct. Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Add support of type set and improve UT

27d2642

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Fix mac prof acc test with wrong NSX version

3491565

Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Improve logging for metadata handling

418dcd6

This change improves logging with: - include filename and line for log print in metadata pkg - switch to TRACE - add context of the struct being handled Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

Fix empty slice assigned as nil

fd9ae7d

If tf schema is updated to empty, the sdk obj should be updated to empty slice instead of nil slice, so that it will be in the PATCH call to remove and clear the list on NSX Signed-off-by: Shawn Wang <shawn.wang@broadcom.com>

wsquan171 force-pushed the test-reflect branch from fe820f1 to cffdca2 Compare May 30, 2024 01:22

wsquan171 force-pushed the test-reflect branch from cffdca2 to a6b13ad Compare May 30, 2024 01:36

annakhm approved these changes May 30, 2024

View reviewed changes

wsquan171 merged commit 09cd1f2 into vmware:master May 31, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reflect lib for auto generation #1205

Add reflect lib for auto generation #1205

wsquan171 commented May 7, 2024 •

edited

wsquan171 commented May 7, 2024

wsquan171 commented May 8, 2024

annakhm May 10, 2024

wsquan171 May 21, 2024

annakhm commented May 13, 2024

wsquan171 commented May 21, 2024

annakhm May 22, 2024

wsquan171 May 30, 2024

annakhm May 22, 2024

wsquan171 May 30, 2024

salv-orlando left a comment

salv-orlando May 28, 2024

annakhm May 28, 2024

wsquan171 May 30, 2024

salv-orlando May 28, 2024

wsquan171 May 30, 2024

wsquan171 commented May 30, 2024

Add reflect lib for auto generation #1205

Add reflect lib for auto generation #1205

Conversation

wsquan171 commented May 7, 2024 • edited

Functional

Non-Functional

TBD in a future PR

wsquan171 commented May 7, 2024

wsquan171 commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annakhm commented May 13, 2024

wsquan171 commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salv-orlando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsquan171 commented May 30, 2024

wsquan171 commented May 7, 2024 •

edited