merged: set attributes even if diagnosed as duplicated
crazy-max opened this issue · 2 comments
I was going to open a PR but wanted your input first.
There are some cases where we would still want to set an attribute when merging bodies even if diagnosed as duplicated: main...crazy-max:hcl:fix-merged-justattrs
It changes the merging semantic though but not sure how to achieve the desired behavior without re-implementing the merged bodies logic on our side.
More context in docker/buildx#1062
Thanks!
Hi @crazy-max,
As I think you've seen in digging in to this, the idea of "merging" bodies is not really a well-defined problem. There are many possible interpretations of what "merging" two bodies might mean, and it's difficult to write a totally-unopinionated implementation if the goal is for the result to still implement the hcl.Body API.
MergeBodies was essentially my naive first attempt at this problem, from before I'd appreciated how nuanced it is. Terraform ended up needing its own variant of MergeBodies to implement all of the (partially accidental) emergent behaviors of Override Files from pre-HCL2 versions of Terraform for example, although Terraform does also use hcl.MergeBodies in a few spots that have different requirements.
I think my main observation here, then, would be that MergeBodies is intentionally implemented only in terms of the same public hcl.Body API that you can call externally, which means that there's no strong reason why it needs to be part of package hcl -- that's just the place that this initial naive implementation happened to end up. If your project would benefit from tighter control over the merging behavior then I think having your own implementation of this logic is a perfectly reasonable thing to do, just as Terraform did: a goal of the syntax-agnostic HCL API is to allow for these sorts of adapters to be created, and for them to appear "invisible" to the calling application.
With all of that said, I'm not totally opposed to changing the MergeBodies behavior but it does seem like you have a different goal than I had in mind when I originally wrote this.
I guess your intent is to behave more like Terraform's "override files" where it's actually expected and therefore not an error at all for a later body to override attributes from an earlier one. It seems like your intent is to just ignore the error diagnostics that MergeBody produces and use the result anyway, but your caller would have no way to distinguish this particular error from other errors such as there being a missing required argument.
Given all of that, my instinct is that it would be better for you to have your own MergeBodies implementation that behaves in the way your application needs, rather than trying to goad the upstream one into doing something it wasn't designed for.
The implementation used for Terraform's override files might be instructive since it was written with similar requirements in mind, although I should point out that the code I linked to above is from a commit that comes after the MPL-to-BSL license change and so you may prefer to refer to exactly the same code from an earlier tag (this code hasn't changed for a loooong time).
In particular here the logic explicitly lets attributes from the second body override attributes in the first body without any explicit checks for duplicates:
I don't know if nested blocks are relevant to your particular problem, but if they are then I'd suggest also comparing how these two implementations define "merging" with regard to nested blocks to see if either of them matches your expectations better than the other. (I don't actually recall how different they are.)
Hey @apparentlymart!
Terraform ended up needing its own variant of
MergeBodiesto implement all of the (partially accidental) emergent behaviors of Override Files from pre-HCL2 versions of Terraform for example, although Terraform does also usehcl.MergeBodiesin a few spots that have different requirements.
Yes it seems the Terraform implementation is similar to the behavior we expect with Buildx bake (related to docker/buildx#1025).
I think my main observation here, then, would be that
MergeBodiesis intentionally implemented only in terms of the same publichcl.BodyAPI that you can call externally, which means that there's no strong reason why it needs to be part ofpackage hcl-- that's just the place that this initial naive implementation happened to end up. If your project would benefit from tighter control over the merging behavior then I think having your own implementation of this logic is a perfectly reasonable thing to do, just as Terraform did: a goal of the syntax-agnostic HCL API is to allow for these sorts of adapters to be created, and for them to appear "invisible" to the calling application.
Ok that makes sense and seems entirely justifiable to create our own implementation of this logic, much like Terraform did. Maybe having this implementation in a package contrib would have been better but thanks for sharing this insight!
but your caller would have no way to distinguish this particular error from other errors such as there being a missing required argument.
We don't have any kind of validation atm so it should be good but we would like to do it in the future (see docker/buildx#491 (comment)), thanks for pointing this out.
I don't know if nested blocks are relevant to your particular problem, but if they are then I'd suggest also comparing how these two implementations define "merging" with regard to nested blocks to see if either of them matches your expectations better than the other. (I don't actually recall how different they are.)
No we don't have this kind of implementation yet on our side.
Thanks for the detailed explanations, it addresses the issue and we will go ahead with our own variant.