starhawking / python-terrascript

Create Terraform files using Python scripts.
BSD 2-Clause "Simplified" License
515 stars 76 forks source link

Change to .update() in 0.8.0? #98

Closed jbscare closed 4 years ago

jbscare commented 4 years ago

In our Terrascript 0.6.1 scripts, we have a bunch of functions that define pieces of the configuration, and we assemble them like this:

ts = terrascript.Terrascript()
ts += terrascript.terraform(
  backend = terrascript.backend(
    "s3",
    **config['backend']['s3']
    )
  )
ts.update(tsumami.provider.define(config))
ts.update(tsumami.s3.define(config))
ts.update(tsumami.security_groups.define(config))

where config is a dict based on a collection of YAML config files.

Each of those functions looks like this:

def define(config):
  my_ts = terrascript.Terrascript()
  if 's3-bucket' in config:
    for (resource, data) in config['s3-bucket'].items():
      # code that does various things
      my_ts += terrascript.resource.aws.aws_s3_bucket(
        resource,
        bucket = bucketname,
        **data
        )
  return(my_ts)

That used to work in 0.6.1, but doesn't in 0.8.0; it seems that each ts.update(something) replaces the previous one, and only the last one ends up in the config when we write out str(ts) at the end.

What's the right way to do this in 0.8.0?

mjuenema commented 4 years ago

Hi Josh, I always feared that I would break something in 0.8.0. I am going to investigate this later today.

mjuenema commented 4 years ago

Hi Josh, sorry for the delay. I don't have a solution yet but can at least offer a detailed analysis: https://github.com/mjuenema/python-terrascript/blob/issue98/docs/issues/098.ipynb (I hope the link works)

mjuenema commented 4 years ago

And added an idea for a possible solution.

jbscare commented 4 years ago

Thanks! Also, if this pattern isn't recommended at this point, it wouldn't be hard to change our script to do something else. We like being able to create multiple Terrascript objects and merge them together, but we could potentially do something else.

mjuenema commented 4 years ago

As it used to work I should probably retain the feature. I have no opinion how Terrascript is meant to be used the right way.

mjuenema commented 4 years ago

Josh, can you please verify whether the update() implementation in branch issue98 would fix your problem.

jbscare commented 4 years ago

Looks good! At least so far -- I've updated a bunch of our modules, enough to get one of our environments to complete successfully, and all it says now is

+$ git diff -U1 tsumami.tf.json
diff --git a/analytics-gold-useast1/misc/tsumami.tf.json b/analytics-gold-useast1/misc/tsumami.tf.json
index 8f7a142..5ba0be7 100644
--- a/analytics-gold-useast1/misc/tsumami.tf.json
+++ b/analytics-gold-useast1/misc/tsumami.tf.json
@@ -2,4 +2,4 @@
   "provider": {
-    "aws": {
-      "__DEFAULT__": {
+    "aws": [
+      {
         "assume_role": {
@@ -11,3 +11,3 @@
       },
-      "us-east-1": {
+      {
         "alias": "us-east-1",
@@ -20,3 +20,3 @@
       }
-    }
+    ]
   },

which I think is expected! (And before it was missing lots of stuff.)

jbscare commented 4 years ago

I've now updated all of our modules, and everything seems to work, and the only changes in the JSON file are like the ones I pasted above. (All this trouble for that little change? Sheesh. :^ )

Looking forward to a 0.8.1 release!

mjuenema commented 4 years ago

Good, i got the __iter__() method out of it as a side-effect :-)