interpolation variable #81

tormath1 · 2020-01-07T11:03:09Z

In this PR:

I added a function Interpolate with two args: the HCL writer (since there's no interpolation with state AFAIK) and an interpolation map which map a value with its interpolation name. (<resource_type>.<resource_name>.(id|self_link))

Feel free to test it.

example of output:

security_groups = ["sg-8a5124ec", "sg-01514a64", "${aws_security_group.Xiskc.id}", "${aws_security_group.inuev.id}"]

⚠️ This is TF0.11 format (#82) ⚠️

Close #37

xlr-8 · 2020-01-07T12:04:27Z

provider/import.go

@@ -130,6 +134,16 @@ func Import(ctx context.Context, p Provider, hcl, tfstate writer.Writer, f *filt
 						return errors.Wrapf(err, "error while calculating the satate of resource %q", t)
 					}
 				}
+				// we construct a map[string]string to perform
+				// interpolation later. By default, the value to interpolate is the ID
+				// but in some case (ex: google), it can be different.


Different how?

actually the comment is misleading. I modified it consequently.

it can be different

resource key could be self_link with Google.

xlr-8 · 2020-01-07T12:17:32Z

provider/resource.go

@@ -35,6 +35,13 @@ type Resource interface {
 	// Type is the type of resource (ex: aws_instance)
 	Type() string

+	// InstanceState is the Terraform state of the resource
+	// it contains important elements likes `Attributes`


xescugc

Overall it's ok, I've commented some changes and also some divagations over the implementation haha.

xescugc · 2020-01-08T14:47:14Z

provider/import.go

+	// google special case
+	// sometimes, the resource is referred by its self_link (https://...)
+	// so we need to identify if we reference the ressource by its ID or its selfLink (the key)


Better add it to the actual comment of the function.

xescugc · 2020-01-08T15:02:10Z

provider/import.go

+		if _, ok := hcl.(*h.Writer); ok {
+			err := util.Interpolate(hcl.(*h.Writer), interpolation)
+			if err != nil {
+				return errors.Wrapf(err, "error while Interpolate Config")
+			}
+		}


When I was reading the util.Interpolate I already thought that it should go to the hcl pkg, and now even more so the implementation is the one that tries to convert to the specific type even with that I don't like that as the Interface was ment to avoid knowing the implementation behind it, and this brakes it.

Ways to solve this:

Have the util be the one that converts so it expects a write.Writer not an hcl.Writer (still not good IMO)

Move it to hcl pkg

Add it to the write.Wiriter interface

Make the Sync() expect the map[string]string.

The last 2 are the ones I prefer as they delay the implementation to the actual one (State will do nothing as no Interpolation is needed and HCL will do something).

xescugc · 2020-01-08T15:10:52Z

provider/import.go

+	if _, ok := s.Attributes["self_link"]; ok {
+		return "self_link"
+	}
+	return "id"


I've commented it to @tormath1 on PM that this could be much more things than just the id, on an aws_instance the relations could be done with https://www.terraform.io/docs/providers/aws/r/instance.html#attributes-reference any of those. But maybe for now only the ID it's the important one.

Among the list we want for sure - and that we already widely use (some services I can think of):
id (general reference), arn (iam,alb,etc), name (iam,r53), fqdn (r53)

Well, my current thought would be to have an AttributesReference associated to a Resource. This attribute would be simply feed by https://github.com/cycloidio/tfdocs.

Interpolation would now be made there:

terracognita/provider/import.go

Line 92 in 5709caa

for i, re := range resources {

since we have all the resources.

Then looping on the resources, if a value of resources.AttributesReferences[*] match with a value in the resources, we interpolate.
WDYT ?

Impletemented: 9460151

xescugc · 2020-01-08T15:11:18Z

provider/import.go

@@ -65,6 +67,8 @@ func Import(ctx context.Context, p Provider, hcl, tfstate writer.Writer, f *filt
 	fmt.Fprintf(out, "Importing with filters: %s", f)
 	logger.Log("filters", f.String())

+	interpolation := make(map[string]string)


Add documentations on what it holds and why is it used.

xescugc · 2020-01-08T15:32:45Z

provider/import.go

+				state := r.InstanceState()
+				if state != nil {
+					key := getKey(state)
+					value := state.Attributes[key]
+					interpolatedValue := fmt.Sprintf("${%s.%s.%s}", r.Type(), r.Name(), key)
+					interpolation[value] = interpolatedValue
+				}


~~Also all this logic could be done on the r.HCL for example as it has all the logic and the actual scope for all this information (state and Name and all of them)~~

Maybe better have a method on the Resource that actually returns all the possible interpolated values (right now just ID but potentially more) as the Resource has access to the schema and could potentially know them.

But maybe for now it's ok this way.

tormath1 · 2020-01-13T10:25:24Z

@xescugc ~~ready for review !~~

I'm not really convinced by this solution and it increases the running time. On master: 2m8s to fetch 10 instances and 21 SG, on this branch: 2m44s for the same thing.

tormath1 · 2020-02-11T17:28:35Z

@xescugc ready for review.

In the latest commit, I totally modified the way I was updating the resources. At the beginning I was trying to simply bytes.ReplaceAll but it's clearly not enough, since we are going to replace also custom tags (e.g cycloid.io was always interpolated), it's also not enough to avoid interpolation with the resource itself (let's suppose you are inside aws_instance.toto you can't refer to aws_instance.toto.some_field from this resource) or cyclic interpolation.

To summarize:

we are now using tfdocs: e7234db in order to get all potentials key to be interpolated.
we are walking recursively into a resources interface in order to interpolate the value whom are able to be interpolated (no self reference and no cyclic reference.)

I tried out with a large infra (security groups and instances) and it works as expected.

xescugc

LGTM it's a good implementation.

xescugc · 2020-02-12T10:40:40Z

hcl/writer.go

+			src := reflect.ValueOf(block)
+			// this will store the updated block
+			dest := reflect.New(src.Type()).Elem()
+			// walk through the resources to interpolate the good values
+			walk(dest, src, i, name, rt)
+			// remove reflect.Value wrapper from dest
+			resources.(map[string]map[string]interface{})[rt][name] = dest.Interface()


Could you space up this part a bit? Comment+Code+\n so it's easy to read if not it's too dense.

xescugc · 2020-02-12T10:41:51Z

hcl/writer.go

+// walk through a resource block. it's easier since we do not how the block is made
+func walk(dest, src reflect.Value, interpolate map[string]string, name string, resourceType string) {


Improve the docs, what does it actually do? It goes though all the src and put it on dest, so instead of "returning" the result is the "dest". (better wording ofc hehe).

xescugc · 2020-02-12T10:42:57Z

hcl/writer.go

+
+	// what we want to interpolate is a string
+	// we do not interpolate a custom tag (like cycloid.io) since it's key.
+	case reflect.String:


Commented on PM with you already but to keep the record, could it be anything else than String no? Int, bool etc.

xescugc

IDK why I forgot to review that code/commit.

xescugc · 2020-02-13T20:45:37Z

provider/resource.go

+		res, err = awsdocs.GetResource(r.resourceType)
+	case "google":
+		res, err = googledocs.GetResource(r.resourceType)
+	}


Return the errors first, basically check them after the end of the switch

xescugc · 2020-02-13T20:47:49Z

provider/resource.go

+	case "aws":
+		res, err = awsdocs.GetResource(r.resourceType)
+	case "google":
+		res, err = googledocs.GetResource(r.resourceType)
+	}


This could also be done by having an static map where the key is the r.provider.String() and the value func (r string) (*tfdocresource.Resource, error), this way is an static map, and easy to add more instead of having to add a new case to the switch.

@xescugc good catch ! it's way cleaner (c7daac0). Ready for final review 😉

xescugc

LGTM. Rebase ans squash the commits that you want.

xlr-8 · 2020-02-18T09:31:07Z

hcl/writer.go

+			resources.(map[string]map[string]interface{})[rt][name] = dest.Interface()
+		}
+	}
+	return nil


Why return an error, if it never will?

Yeh it could be removed now as it's using a different logic.

xlr-8 · 2020-02-18T09:31:43Z

hcl/writer.go

+	return nil
+}
+
+// walk through a resource block. it's easier since we do not how the block is made


do not know how* ?

xescugc

For the commits:

This 42dfe1d, a99de04 and c9751f1 change the same logic 3 times, a logic that was added on this PR clean it a bit :)
abcf2e2 try to split the go.mod|sum in a different commit just to not pollute commits, as it's hard to read then.

xescugc · 2020-02-18T11:28:41Z

provider/resource.go

+	providerResources           = map[string]func(string) (*tfdocs.Resource, error){
+		"aws": func(rt string) (*tfdocs.Resource, error) {
+			return awsdocs.GetResource(rt)
+		},
+		"google": func(rt string) (*tfdocs.Resource, error) {
+			return googledocs.GetResource(rt)
+		},
+	}


Ok a better way:

Define a function type func(rt string) (*tfdocs.Resource, error)

Then the map just have to have "google" googledocs.GetResource

Which make is much more simpler (that was my first intention but IDK why I read it wrong on the implementation).

Or actually the way you have it now you can still directly use the format without defining the function actually 😄

xescugc · 2020-02-18T11:31:22Z

hcl/writer.go

+			resources.(map[string]map[string]interface{})[rt][name] = dest.Interface()
+		}
+	}
+	return nil


Yeh it could be removed now as it's using a different logic.

we need to be able to get this name in order to construct the complete path of the resource <type>.<name>.<key>

tormath1 · 2020-02-19T16:31:39Z

@xescugc final rebase has been made :)

xescugc

Just one comment :) Rebase it directly.

xescugc · 2020-02-20T10:17:30Z

provider/resource.go

@@ -119,6 +132,21 @@ func NewResource(id, rt string, p Provider) Resource {
 	}
 }

+func (r *resource) AttributesReference() ([]string, error) {
+	resourceFunc := providerResources[r.provider.String()]


Check the ok also, as it could not exists.

@xescugc done in 6fbac85 :)

this function will be used to identify which fields have the ability to be interpolated. For each resource, the attributes will be provided by tfdocs

we first iterate on block resources then for each block we walk through it and for each isolated string, we try to find its interpolation value from the `interpolate` array.

tormath1 self-assigned this Jan 7, 2020

xlr-8 reviewed Jan 7, 2020

View reviewed changes

tormath1 force-pushed the mt-add-interpolation branch from b7ac476 to e678986 Compare January 7, 2020 13:07

tormath1 added Status: Need review Type: Enhancement Provide a new feature or improve an existing one Status: In progress and removed Status: Need review labels Jan 7, 2020

xescugc requested changes Jan 8, 2020

View reviewed changes

tormath1 added Status: Need review Status: In progress Status: Under discussion Must be discussed for determining what to do and removed Status: In progress Status: Need review labels Jan 10, 2020

tormath1 force-pushed the mt-add-interpolation branch 2 times, most recently from c7b6303 to 2f3e972 Compare January 13, 2020 10:15

tormath1 added Status: Need review and removed Status: In progress Status: Under discussion Must be discussed for determining what to do labels Jan 13, 2020

tormath1 added Status: In progress and removed Status: Need review labels Jan 17, 2020

tormath1 force-pushed the mt-add-interpolation branch 3 times, most recently from 808c787 to bdd2262 Compare February 11, 2020 17:21

tormath1 force-pushed the mt-add-interpolation branch from 01587da to bb2cd72 Compare February 11, 2020 17:37

tormath1 added Status: Need review and removed Status: In progress labels Feb 11, 2020

xescugc requested changes Feb 12, 2020

View reviewed changes

xescugc requested changes Feb 13, 2020

View reviewed changes

xescugc reviewed Feb 14, 2020

View reviewed changes

tormath1 force-pushed the mt-add-interpolation branch 3 times, most recently from 7dc9363 to 6689ba3 Compare February 18, 2020 09:17

xlr-8 reviewed Feb 18, 2020

View reviewed changes

xescugc requested changes Feb 18, 2020

View reviewed changes

tormath1 added 2 commits February 19, 2020 16:29

resource: get Name generated by TC

b84bd1f

we need to be able to get this name in order to construct the complete path of the resource <type>.<name>.<key>

provider: fix typo on instanceResource

fe0ab0b

tormath1 force-pushed the mt-add-interpolation branch from 6689ba3 to 891d5d2 Compare February 19, 2020 16:28

xescugc requested changes Feb 20, 2020

View reviewed changes

tormath1 force-pushed the mt-add-interpolation branch 3 times, most recently from 9ac8fef to 92fe28e Compare February 20, 2020 13:36

tormath1 added 4 commits February 20, 2020 14:36

provider: add AttributesReference function

796d052

this function will be used to identify which fields have the ability to be interpolated. For each resource, the attributes will be provided by tfdocs

chore: add tfdocs dependency

e136155

write: use reflect to walk through resources

c794e1d

we first iterate on block resources then for each block we walk through it and for each isolated string, we try to find its interpolation value from the `interpolate` array.

chore: add changelog entry

92fe28e

xescugc approved these changes Feb 20, 2020

View reviewed changes

tormath1 merged commit dfc5a99 into master Feb 20, 2020

tormath1 deleted the mt-add-interpolation branch February 20, 2020 14:02

xlr-8 removed the Status: Need review label Feb 20, 2020

tormath1 mentioned this pull request Jul 31, 2020

tfstate: add dependencies #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interpolation variable #81

interpolation variable #81

tormath1 commented Jan 7, 2020 •

edited

xlr-8 Jan 7, 2020

tormath1 Jan 7, 2020

xlr-8 Jan 7, 2020

xescugc left a comment

xescugc Jan 8, 2020

xescugc Jan 8, 2020

xescugc Jan 8, 2020

xlr-8 Jan 16, 2020

tormath1 Jan 17, 2020 •

edited

xescugc Jan 8, 2020

xescugc Jan 8, 2020

tormath1 commented Jan 13, 2020 •

edited

tormath1 commented Feb 11, 2020 •

edited

xescugc left a comment •

edited

xescugc Feb 12, 2020

xescugc Feb 12, 2020

xescugc Feb 12, 2020

xescugc left a comment

xescugc Feb 13, 2020

xescugc Feb 13, 2020

tormath1 Feb 14, 2020 •

edited

xescugc left a comment

xlr-8 Feb 18, 2020

xescugc Feb 18, 2020

xlr-8 Feb 18, 2020

xescugc left a comment

xescugc Feb 18, 2020

xescugc Feb 18, 2020

tormath1 commented Feb 19, 2020

xescugc left a comment

xescugc Feb 20, 2020

tormath1 Feb 20, 2020

		// walk through a resource block. it's easier since we do not how the block is made
		func walk(dest, src reflect.Value, interpolate map[string]string, name string, resourceType string) {

interpolation variable #81

interpolation variable #81

Conversation

tormath1 commented Jan 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xescugc 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

tormath1 Jan 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tormath1 commented Jan 13, 2020 • edited

tormath1 commented Feb 11, 2020 • edited

xescugc left a comment • edited

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

xescugc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tormath1 Feb 14, 2020 • edited

Choose a reason for hiding this comment

xescugc 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

xescugc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tormath1 commented Feb 19, 2020

xescugc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tormath1 commented Jan 7, 2020 •

edited

tormath1 Jan 17, 2020 •

edited

tormath1 commented Jan 13, 2020 •

edited

tormath1 commented Feb 11, 2020 •

edited

xescugc left a comment •

edited

tormath1 Feb 14, 2020 •

edited