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

Added support for #14

Closed
wants to merge 1 commit into from
Closed

Added support for #14

wants to merge 1 commit into from

Conversation

llyys
Copy link

@llyys llyys commented Sep 7, 2020

  • standard functions
attrs = jsonencode({foo = 'bar'})
  • negative numbers
y = -10

- standard functions
- negative numbers
@tmccombs
Copy link
Owner

tmccombs commented Sep 8, 2020

The negative number part of this, I think is great.

The standard functions part, I'm not as sure about. As I understand this, the change is actually evaluating the function during the conversion to json, which can lose some of the original information (like the dependencies on the arguments to the function). It also only works for certain functions, which, for example doesn't work for all functions supported by terraform, and I think would cause this to error instead of translating to hcl-compatible json.

I'm curious what your use-case is for evaluating functions at translation time? I'd also be more ok with it if it was behind a flag.

@tmccombs tmccombs added enhancement New feature or request open questions labels Sep 8, 2020
@llyys
Copy link
Author

llyys commented Sep 8, 2020

The problem is that terraform does not support nested maps
hashicorp/terraform-plugin-sdk#62
So we are overcoming this with encoding the payload as json string

resource "statechart" "default" {
  name = "Simple chart"
  definition = jsonencode({
     type="start"
     properties={
        label="Start"
        offset={
            x=10
            y=-10
        } 
     }
  })
}

As these functions are part of standard TF functions I thought it's nice to have those things

@tmccombs
Copy link
Owner

So, currently if you have something like attrs = jsonencode({foo="foo"}), then hcl2json will encode that as:

"attrs": "${jsonencode({foo=\"foo\"})}"

And terraform can handle this syntax just fine.

It sounds like you want it to be encoded as:

"attrs": "{\"foo\":\"foo\"}"

I'm still not sure what the use case for that is.

I also don't want to accept this PR as is, because if there is a function hcl2json doesn't know about, or it can't evaluate an argument to a function (for example because it uses a reference to another resource in terraform), then it will fail. I might accept it if hcl2json gracefully falls back to the current behavior if it is unable to successfully evaluate the function, especially if there is a flag to toggle the behavior.

@tmccombs
Copy link
Owner

I added the negative number support in e4347db

@@ -90,10 +111,15 @@ func (c *converter) convertExpression(expr hclsyntax.Expression) (interface{}, e
switch value := expr.(type) {
case *hclsyntax.LiteralValueExpr:
return ctyjson.SimpleJSONValue{Value: value.Val}, nil
case *hclsyntax.UnaryOpExpr:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality is already on master.

}
value, valueDiags := expr.Value(specCtx)
if valueDiags.HasErrors() {
return nil, fmt.Errorf(valueDiags.Error())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fall back to the existing behavior of wrapping the expression.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's not that important.

@llyys llyys closed this Sep 12, 2020
@tmccombs
Copy link
Owner

b59e3a3 may interest you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request open questions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants