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

generated schema disallows reference strings #70

Open
KiaraGrouwstra opened this issue Dec 25, 2024 · 6 comments
Open

generated schema disallows reference strings #70

KiaraGrouwstra opened this issue Dec 25, 2024 · 6 comments

Comments

@KiaraGrouwstra
Copy link

Describe the bug

currently, tf-ncl seems to generate straightforward types (e.g. Number), which ends up not matching up with how in practice one should also be able to pass reference strings:

error: contract broken by the value of `server_id`
       ┌─ ./tf-ncl-schema.ncl:107854:15
       │
107854 │             | TfNcl.Tf Number
       │               --------------- expected type
       │
...
       ┌─ <unknown> (generated by evaluation):1:1
       │
     1 │ "${hcloud_server.my_server.id}"
       │ ------------------------------ evaluated to this value

To Reproduce

  1. have a TF value that should be a number
  2. pass a reference string, like "${hcloud_server.my_server.id}"
  3. find tf-ncl disallowing such a string value

Expected behavior

probably any TF type should also accept strings (formatted like references)

Environment

  • OS name + version: nixos 24.11
  • Version of the code: git

Additional context

The TF docs describe the intended functioning on this as follows:

If the given template consists only of a single interpolation sequence, the result of its expression is taken directly, without first converting it to a string. This allows non-string expressions to be used within the JSON syntax:

{
  "output": {
    "example": {
      "value": "${aws_instance.example}"
    }
  }
}

The output "example" declared above has the object value representing the given aws_instance resource block as its value, rather than a string value. This special behavior does not apply if any literal or control sequences appear in the template; in these other situations, a string value is always produced.

@yannham
Copy link
Member

yannham commented Dec 26, 2024

So, I must admit I haven't been following tf-ncl for quite some time, so my knowledge is a bit rusty. But I think the auto-generated schema automatically populate some computed fields in Nickel, that are special values with a marker (probably today they should just be ADTs aka enum variants, but at the time of developing tf-ncl, those didn't exist in Nickel). They are post-processed by tf-ncl to be exported as terraform interpolation.

Thus, I think you should be able to reference ids directly in Nickel without having to care about it being computed and Terraform interpolation, just as {foo = hcloud_server.my_server.id}, and the post-processing step will do its magic (and, indeed, the Tf contract does have a special case for those encoded computed values).

That being said, there are some limitations and not all computed values can be used this way (in particular tf-ncl doesn't support . I thus agree that to be more flexible we should also accept plain interpolated strings, although we lose some validation power, because we wouldn't check anymore that the computed field is indeed a number. Ideally we would check as well, but that doesn't seem easy at all, and forbidding custom interpolations in the meantime might prove too restrictive in practice.

@yannham
Copy link
Member

yannham commented Dec 26, 2024

(I hereby invoke @vkleen to correct me if I said something wrong)

@vkleen
Copy link
Contributor

vkleen commented Dec 26, 2024

That's right, the design intent was that you wouldn't have to use Terraform's interpolation. Instead, it should be possible (and easier! Not least because of LSP completion) to just recursively reference fields and have things work magically. Unfortunately, the current setup doesn't support complex interpolations and maybe it would be a good idea to offer an escape hatch. Maybe Tf.raw "${...}" could be made to work with some tweaks to the contracts.

@KiaraGrouwstra
Copy link
Author

in particular tf-ncl doesn't support .

@yannham i think your sentence is missing a bit there? 🙈

the workaround i had trouble with for a few reasons:

  • lack of documentation (Better introductory documentation #13, readme documentation link broken #71)
  • being new to the underlying concepts, having referenced such an id from a let-binding that did not yet have scope over the terraform info*
  • use dynamic references to reference TF variables*
  • use nix run-time transforms to construct the TF structure*
  • referenced attributes (e.g. the mentioned server id) only being assigned by a remote service at TF run-time, i.e. not set/existent in the Nickel code for a Nickel 'static interpolation' (e.g. hcloud_server.my_server.id) to reference. in other words, the workaround seems viable for TF arguments, tho not so much for TF attributes, like hcloud_server's id.

* these were on my coding style, coming from nix

It looks like mostly the last one seems not to have workarounds still.

Maybe Tf.raw "${...}" could be made to work with some tweaks to the contracts.

@vkleen That would seem great - sounds like that should address any of these.

@yannham
Copy link
Member

yannham commented Dec 28, 2024

@yannham i think your sentence is missing a bit there? 🙈

Oops, I meant in particular tf-ncl doesn't support compound expressions involving TF computed values, like taking the first byte of an IP address, which TF does handle to some extent if I recall correctly.

@vkleen That would seem great - sounds like that should address any of these.

That sounds fair. I wonder if maybe we don't even need to modify the original Tf contract which already accommodates interpolated fields, but just exposing like

raw_computed_reference | Array String -> Dyn = fun path_ =>
  {
    path = path_,
    terraform_field_type = 'ProviderComputed,
  }

To be used as TfNcl.raw_computed_reference ["hcloud", "myserver", "id"] (or we could take a string and split on .).

@KiaraGrouwstra
Copy link
Author

@yannham thanks, i think your suggested function sounds great. i guess we could add both versions, as they seem to offer low-level vs high-level interfaces respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants