TF Plan to ACI Payload Converter (DCNE-164) #1276

harismalk · 2024-08-22T04:35:59Z

Fixes #586

codecov-commenter · 2024-08-25T19:49:49Z

Codecov Report

Attention: Patch coverage is 76.53631% with 84 lines in your changes missing coverage. Please review.

Project coverage is 84.85%. Comparing base (176a6cb) to head (1c80161).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...provider/resource_aci_access_interface_override.go	71.42%	2 Missing ⚠️
...nternal/provider/resource_aci_custom_qos_policy.go	71.42%	2 Missing ⚠️
...rovider/resource_aci_data_plane_policing_policy.go	71.42%	2 Missing ⚠️
internal/provider/resource_aci_endpoint_tag_ip.go	71.42%	2 Missing ⚠️
internal/provider/resource_aci_endpoint_tag_mac.go	71.42%	2 Missing ⚠️
...ovider/resource_aci_epg_useg_ad_group_attribute.go	71.42%	2 Missing ⚠️
.../provider/resource_aci_epg_useg_block_statement.go	71.42%	2 Missing ⚠️
...al/provider/resource_aci_epg_useg_dns_attribute.go	71.42%	2 Missing ⚠️
...nal/provider/resource_aci_epg_useg_ip_attribute.go	71.42%	2 Missing ⚠️
...al/provider/resource_aci_epg_useg_mac_attribute.go	71.42%	2 Missing ⚠️
... and 33 more

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1276   +/-   ##
=======================================
  Coverage   84.85%   84.85%           
=======================================
  Files          95       95           
  Lines       34992    34992           
=======================================
  Hits        29694    29694           
  Misses       3936     3936           
  Partials     1362     1362

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shrsr

Can you briefly

README.md

shrsr · 2024-08-26T18:30:39Z

README.md


+To export a Terraform Plan as an ACI Payload:
+
+1. Navigate to conversion directory 


It would be nice to run the code without changing the working directory to conversion

cmd/conversion/main.go

README.md

shrsr · 2024-08-27T19:42:35Z

cmd/conversion/main.go

+}
+
+func traverseOrCreatePath(dnMap map[string]*TreeNode, rootNode *TreeNode, resourceType, dn string) *TreeNode {
+	pathSegments := strings.Split(dn, "/")


This is likely not enough to handle complex dn patterns. There's a function in our provider that could be utilized here. Let me look for it.

akinross · 2024-08-30T07:20:56Z

cmd/conversion/main.go

+	} `json:"change"`
+}
+
+func runTerraform() (string, error) {


would it be possible to have this as a method on the Plan struct? where the method is called something like getPlanOutput

akinross · 2024-08-30T07:21:19Z

cmd/conversion/main.go

+	planBin := "plan.bin"
+	planJSON := "plan.json"


should we pass these to the function as arguments?

akinross · 2024-08-30T07:24:21Z

cmd/conversion/main.go

+	return planJSON, nil
+}
+
+func readPlan(jsonFile string) (Plan, error) {


is there a reason why you separated this from the runTerraform() ?

akinross · 2024-08-30T07:25:28Z

cmd/conversion/main.go

+	var plan Plan
+	data, err := os.ReadFile(jsonFile)
+	if err != nil {
+		return plan, fmt.Errorf("failed to read input file: %w", err)
+	}
+
+	if err := json.Unmarshal(data, &plan); err != nil {


Suggested change

var plan Plan

data, err := os.ReadFile(jsonFile)

if err != nil {

return plan, fmt.Errorf("failed to read input file: %w", err)

}

if err := json.Unmarshal(data, &plan); err != nil {

data, err := os.ReadFile(jsonFile)

if err != nil {

return plan, fmt.Errorf("failed to read input file: %w", err)

}

var plan Plan

if err := json.Unmarshal(data, &plan); err != nil {

akinross · 2024-08-30T07:29:20Z

cmd/conversion/main.go

+		payload := createFunc(values, status)
+		return payload
+	}
+	return nil


Should we log something or provide some alternative action when the resourceType does not exist?

akinross · 2024-08-30T08:10:27Z

cmd/conversion/main.go

+}
+
+func generateUniqueKeyForNonDnNode(resourceType string, attributes map[string]interface{}) string {
+	return fmt.Sprintf("%s-%v", resourceType, attributes["name"])


what if attributes does not contain name?

akinross · 2024-08-30T08:11:47Z

cmd/conversion/main.go

+
+			childKey := childDn
+			if childDn == "" {
+				childKey = generateUniqueKeyForNonDnNode(childClassName, childAttributes)


what is a non dn node? a rn without identifier?

akinross · 2024-08-30T08:14:03Z

cmd/conversion/main.go

+
+	classNames := parseClassNames(pathSegments, resourceType)
+
+	for i := 1; i < len(pathSegments); i++ {


why not start loop with 0 then classnames can just be indexed with i?

akinross · 2024-08-30T08:19:50Z

dict/dict.go

+func GetDnToAciClassMap(childClass string, parentPrefix string) string {
+	rnMapping := make(map[string]map[string]string)
+
+	resp, err := http.Get("https://pubhub.devnetcloud.com/media/model-doc-latest/docs/doc/jsonmeta/aci-meta.json")


So every time this function is called we are retrieving the full meta?

Should we do this one time and cache it in memory or store it as static file in the repo?

As static file we might want because what happens when the meta from devnet deviates from the meta information we are using in the generator? Other option is to create a template in the generator that construct the information you need here.

akinross · 2024-08-30T08:23:57Z

cmd/conversion/main.go

+		if pathSegments[i] == "uni" {
+			break
+		}
+		className := dict.GetDnToAciClassMap(classNames[len(classNames)-1], prefix)


see comment in the function itself

Haris Malik added 3 commits August 21, 2024 14:00

[ignore] added template

bfc2e9b

[ignore]

5d04147

[minor_changes] Revised converter and fixed legacy documentation

b6a37a6

lhercot requested review from samiib and sajagana August 22, 2024 15:53

[minor_changes] Revised converter, templates, and documentation

c15ec82

Haris Malik added 3 commits August 25, 2024 14:48

[ignore]

13934c5

[ignore]

6af4375

[ignore]

4a6e0c5

lhercot requested a review from akinross August 26, 2024 15:25

shrsr requested changes Aug 27, 2024

View reviewed changes

Haris Malik added 3 commits August 28, 2024 15:35

[minor_changes] added test and revised documentation

23d0e1a

[ignore]

95c8bd0

[ignore]

1c80161

harismalk changed the title ~~Revised converter and documentation~~ TF Plan to ACI Payload Converter Aug 29, 2024

akinross requested changes Aug 30, 2024

View reviewed changes

akinross changed the title ~~TF Plan to ACI Payload Converter~~ TF Plan to ACI Payload Converter (DCNE-164) Oct 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TF Plan to ACI Payload Converter (DCNE-164) #1276

TF Plan to ACI Payload Converter (DCNE-164) #1276

harismalk commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 25, 2024 •

edited

Loading

shrsr left a comment

shrsr Aug 26, 2024

shrsr Aug 27, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024

akinross Aug 30, 2024


		To export a Terraform Plan as an ACI Payload:

		1. Navigate to conversion directory


		classNames := parseClassNames(pathSegments, resourceType)

		for i := 1; i < len(pathSegments); i++ {

TF Plan to ACI Payload Converter (DCNE-164) #1276

Are you sure you want to change the base?

TF Plan to ACI Payload Converter (DCNE-164) #1276

Conversation

harismalk commented Aug 22, 2024 • edited Loading

codecov-commenter commented Aug 25, 2024 • edited Loading

Codecov Report

shrsr 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

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

Choose a reason for hiding this comment

harismalk commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 25, 2024 •

edited

Loading