Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recursive DeepMerge function #25032

Closed
wants to merge 11 commits into from

Conversation

bruceharrison1984
Copy link

@bruceharrison1984 bruceharrison1984 commented May 25, 2020

Description

This function duplicates the existing behavior of the merge function, and supplements it with recursive scanning to child objects. This fixes scenarios where you may want to do a partial update of a nested object, but the current merge function can't perform it due to it only operating at the root level. Any type can be overridden with a different type by a later map/object.

A new function was created to avoid breaking code that relies on the current merge behavior.

Tests

All the original merge unit tests were copied, and pass against deepmerge. Additional tests were added for overriding child properties.

Example

variable map1 {
    default = {
        a = "test"
        b = {
            b1 = "b1"
            b2 = "b2"
            b4 = {
                b5 = "b5"
            }
        }
        c = [1,2,3]
    }
}

variable map2 {
    default = {
        b = {
            b1 = "b1-updated"
            b3 = "b3"
            b4 = {
                b5 = "b5-updated"
            }
        }
        c = [1,2,3,4]
    }
}

locals{
    output = deepmerge(var.map1, var.map2)
}

output o {
    value = local.output
}
Outputs:

o = {
  "a" = "test"
  "b" = {
    "b1" = "b1-updated"
    "b2" = "b2"
    "b3" = "b3"
    "b4" = {
      "b5" = "b5-updated"
    }
  }
  "c" = [
    1,
    2,
    3,
    4,
  ]
}

@hashicorp-cla
Copy link

@hashicorp-cla hashicorp-cla commented May 25, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

@codecov codecov bot commented May 25, 2020

Codecov Report

Merging #25032 into master will increase coverage by 0.03%.
The diff coverage is 93.10%.

Impacted Files Coverage Δ
lang/funcs/collection.go 80.89% <92.98%> (+2.47%) ⬆️
lang/functions.go 92.68% <100.00%> (+0.05%) ⬆️
terraform/node_resource_plan.go 91.73% <0.00%> (-1.66%) ⬇️

@pdemagny
Copy link

@pdemagny pdemagny commented Jun 8, 2020

Thank you very much !
This function would be pure gold to me, i would use it in all my modules input variables.
Any chance for this to make it into 0.13, or even 0.12.x ?

@adegtyarev
Copy link

@adegtyarev adegtyarev commented Jun 8, 2020

This is a useful gem indeed because it allows you to create compact and portable code when it comes to complex objects. Without it, merging complex objects requires writing nested for expressions. Add to this a mixture of different types, which is very similar to what a typical Terraform resource definition is, and it becomes very specific code for a specific use case. With this nice deepmerge function you have exactly what you need out of the box.

@jbergknoff-rival
Copy link

@jbergknoff-rival jbergknoff-rival commented Jun 24, 2020

Awesome, thank you for your work on this. I played around with it (on commit c16b626) and was able to cause some panics:

values.tf

locals {
  x = { a = "test", b = { b1 = "b1", b2 = "b2", b4 = { b5 = "b5" } }, c = [1,2,3] }
  y = { b = { b1 = "b1-updated", b3 = "b3", b4 = { b5 = "b5-updated" } }, c = [1,2,3,4] }
  bad = { a = { b = 1, c = [3] } }
}

Then, running terraform console in the directory with values.tf:

Working as intended

> deepmerge(local.x, local.y) # same as example in PR description
{
  "a" = "test"
  "b" = {
    "b1" = "b1-updated"
    "b2" = "b2"
    "b3" = "b3"
    "b4" = {
      "b5" = "b5-updated"
    }
  }
  "c" = [
    1,
    2,
    3,
    4,
  ]
}

Panic: "value is not a collection"

> deepmerge(local.x, local.bad)

>  
Error: Error in function call

  on <console-input> line 1:
  (source code not available)
    |----------------
    | local.bad is object with 1 attribute "a"
    | local.x is object with 3 attributes

Call to function "deepmerge" failed: panic in function implementation: value
is not a collection
goroutine 1 [running]:
runtime/debug.Stack(0xc0007d5510, 0x220a960, 0x2c2f480)
	/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
github.com/zclconf/go-cty/cty/function.errorForPanic(...)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/function/error.go:44
github.com/zclconf/go-cty/cty/function.Function.ReturnTypeForValues.func1(0xc0007d5f08,
0xc0007d5f18)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/function/function.go:217
+0x78
panic(0x220a960, 0x2c2f480)
	/usr/local/go/src/runtime/panic.go:969 +0x166
github.com/zclconf/go-cty/cty.Value.LengthInt(0x2cd84c0, 0xc0000503c9,
0x220a960, 0xc0002d7fb0, 0x22f5c01)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/value_ops.go:1012
+0x24d
github.com/zclconf/go-cty/cty.Value.AsValueMap(0x2cd84c0, 0xc0000503c9,
0x220a960, 0xc0002d7fb0, 0xc0007d5be0)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/value_ops.go:1245
+0x90
github.com/hashicorp/terraform/lang/funcs.recursiveMerge(0x2cd85c0,
0xc000126058, 0x22f5ce0, 0xc0005fe690, 0xc0007d5928, 0xc0007d5928)
	/go/src/hashicorp/terraform/lang/funcs/collection.go:230 +0x6ab
github.com/hashicorp/terraform/lang/funcs.glob..func9(0xc000215f80, 0x2, 0x2,
0x40ffc48, 0xc0007d5d40, 0x0, 0x0)
	/go/src/hashicorp/terraform/lang/funcs/collection.go:147 +0x45f
github.com/zclconf/go-cty/cty/function.Function.ReturnTypeForValues(0xc00018bf50,
0xc000215f80, 0x2, 0x2, 0x0, 0x0, 0x0, 0x0)
...

Panic: inconsistent map element types

> deepmerge(local.bad, local.x)

>  
Error: Error in function call

  on <console-input> line 1:
  (source code not available)
    |----------------
    | local.bad is object with 1 attribute "a"
    | local.x is object with 3 attributes

Call to function "deepmerge" failed: panic in function implementation:
inconsistent map element types (cty.Number then
cty.Tuple([]cty.Type{cty.Number}))
goroutine 1 [running]:
runtime/debug.Stack(0xc0004c7488, 0x22ba960, 0xc00046c170)
	/usr/local/go/src/runtime/debug/stack.go:24 +0x9d
github.com/zclconf/go-cty/cty/function.errorForPanic(...)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/function/error.go:44
github.com/zclconf/go-cty/cty/function.Function.ReturnTypeForValues.func1(0xc0004c7f08,
0xc0004c7f18)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/function/function.go:217
+0x78
panic(0x22ba960, 0xc00046c170)
	/usr/local/go/src/runtime/panic.go:969 +0x166
github.com/zclconf/go-cty/cty.MapVal(0xc000570d80, 0xc000126050, 0x22f5ce0,
0xc0005fe630, 0xc000570d80)
	/go/src/hashicorp/terraform/vendor/github.com/zclconf/go-cty/cty/value_init.go:207
+0x4b3
github.com/hashicorp/terraform/lang/funcs.recursiveMerge(0x2cd85c0,
0xc000126058, 0x22f5ce0, 0xc0005fe690, 0xc0004c7928, 0x3f8f6f8)
	/go/src/hashicorp/terraform/lang/funcs/collection.go:234 +0x45e
github.com/hashicorp/terraform/lang/funcs.glob..func9(0xc0005cc3c0, 0x2, 0x2,
0x40ffc48, 0xc0004c7d40, 0x0, 0x0)
	/go/src/hashicorp/terraform/lang/funcs/collection.go:147 +0x45f
github.com/zclconf/go-cty/cty/function.Function.ReturnTypeForValues(0xc00018bf50,
0xc0005cc3c0, 0x2, 0x2, 0x0, 0x0, 0x0, 0x0)

I'll see if I have any luck modifying the implementation to address these.

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Jun 24, 2020

@jbergknoff-rival thanks for the feedback. I'll take a look at that scenario as well. I wouldn't be surprised if I missed a couple edges cases here, I tried to go for as generic an approach as possible

@jbergknoff-rival
Copy link

@jbergknoff-rival jbergknoff-rival commented Jun 24, 2020

@bruceharrison1984 I took a stab at this (diff and new code). There were two problems that I saw:

  • The !oldVal.Type().IsListType() guard is letting things like strings and numbers get through, which crash when we compute objectMap = oldVal.AsValueMap(). This led to the "value is not a collection" panic when doing something like deepmerge({a="hi"},{a={b=1}}).

  • Less easy to fix: the "object or map?" logic in recursiveMerge() is too superficial. It looks at deepmerge({a={b=1, c=[2]}}) and says (1) types match on the top level map, because a is the only key, so (2) output["a"] = cty.MapVal(recursiveMerge(...)). But recursiveMerge() on the inner stuff comes back as it was, with disparate types, and MapVal panics.

I refactored a bit so recursiveMerge() takes in two cty.Values and returns a cty.Value, and the type information is built up recursively in that process (i.e. not computed separately). I also added some tests covering the two panic scenarios.

However, I broke support for dynamic and/or unknown values. I don't understand exactly what these are or how they're handled, so I didn't work more on this. It might be easy to re-add support, for all I know.

Also: there's some ambiguity about how null values should be merged. In this implementation, a null value deletes a key from the merged map. I think this is reasonable because otherwise there's no mechanism to get rid of things. If one wants to leave the value as is, it's possible to omit the key or pass in {} in the case of a map/object.

I hope that's useful. Feel free to use any of this or scrap it.

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Jun 24, 2020

@jbergknoff-rival You beat me to the punch 😆 I had refactored both of the issues you had pointed out, but I think I like your implementation a bit better with the return type detection pulled out into another func. I'll go ahead an incorporate your changes in to what's here.

Looks like only one test is failing, and it's due to UnknownVal's. I also agree with your take on using Nulls to remove values, and empty brackets to keep them. Seems like a simple enough solution.

Your changes have been cherry-picked in, and the failing tests are commented out for now

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Jun 25, 2020

Added back support for dynamic/unknown types. All tests are passing again.

@jbergknoff-rival
Copy link

@jbergknoff-rival jbergknoff-rival commented Jun 25, 2020

Nice! Thanks again. We're going to start incorporating this in our custom build of TF. With any luck, the PR will get some attention from the maintainers 🤞 .

@marlock9
Copy link

@marlock9 marlock9 commented Jul 7, 2020

Did you consider to do this as external provider with data source which outputs result of deepmerging? This way you will be independent from hashicorp PR approval/merging process. Correct me please if i'm wrong.

@phlegx
Copy link

@phlegx phlegx commented Jul 7, 2020

This is totally awesome! We would also need this urgently!

Hope the maintainers merge it soon. Thanks for all your effort!

@phlegx
Copy link

@phlegx phlegx commented Jul 8, 2020

Dear @aicarmic!

Can we add this pull request into the Terraform code. All seems there, even the doc is present and it seems to be functional.

It would save lots of people lot of additional effort. Just like when having for example an overall general config map which one wants to combine/merge with a more specific config map in single workspace config files.

@galindro
Copy link

@galindro galindro commented Jul 10, 2020

Can't wait to see this in the next release... Will it be added to 0.13 only?

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Aug 7, 2020

@jbergknoff-rival would you mind completing the CLA on the off chance this gets merged at some point?

@jbergknoff-rival
Copy link

@jbergknoff-rival jbergknoff-rival commented Aug 7, 2020

Hm, I've actually already signed it. I just had a conversation like this a couple of days ago on hashicorp/go-getter#218 (comment) :). I think it's a glitch in the CLA status thing.

@ilicmilan
Copy link

@ilicmilan ilicmilan commented Aug 27, 2020

Any update on this?

@ScottFinlaysonGMSL
Copy link

@ScottFinlaysonGMSL ScottFinlaysonGMSL commented Sep 3, 2020

@jbergknoff-rival might you need to do the same fix that you applied here? hashicorp/go-getter#218 (comment)

@jbergknoff
Copy link

@jbergknoff jbergknoff commented Sep 3, 2020

@jbergknoff-rival might you need to do the same fix that you applied here? hashicorp/go-getter#218 (comment)

Thanks for the reminder, @ScottFinlaysonGMSL . I don't have push access to the PR branch, but I think @bruceharrison1984 can do it to fix the CLA. Bruce, if you would amend the commits on your branch, made by me, to use the email address found here, then I think the CLA check will pass. Since it's a rewrite of history, it will require force pushing the PR branch.

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Sep 3, 2020

@jbergknoff Looks like it's all fixed up now. Thanks for the heads up

@bgmonroe
Copy link

@bgmonroe bgmonroe commented Sep 24, 2020

Apologies if this is a controversial question but if deepmerge only adds functionality that the current merge function lack (but acts mostly the same), why not just augment this functionality into the merge function? Having a similar but slightly different function seems fiddly at best and confusing at worst. FWIW, I love that this capability is being added.

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Sep 24, 2020

@bgmonroe A new function was created because there could very well be cases where someone took advantage of the fact that the current merge function only operates at the root level. If this outright replaced it, it could cause failures due to the behavior change. If accepted, this could eventually replace the original merge, but I think a phased roll out would give people time to refactor their templates to conform to the new behavior. Thus a new function was born.

@pisto
Copy link

@pisto pisto commented Oct 7, 2020

looking at this.

@braghettos
Copy link

@braghettos braghettos commented Oct 7, 2020

Pushing for this PR! @bruceharrison1984

@KyleKotowick
Copy link

@KyleKotowick KyleKotowick commented Oct 15, 2020

While waiting for that PR to be merged, I built a module to do this in pure Terraform. It seems to be working, but I could use some more testers! https://registry.terraform.io/modules/Invicton-Labs/deepmerge/null/latest

@bruceharrison1984
Copy link
Author

@bruceharrison1984 bruceharrison1984 commented Nov 3, 2020

There is ongoing discussion with HashiCorp around this in the original Issue. I'd suggest posting your thoughts there if you wish to be involved with the discussion.

#24987

Base automatically changed from master to main Feb 24, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Apr 18, 2022

I'm going to lock this pull request because it has been closed for 30 days . This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet