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

[proposal] Introduce Schema.TypeJSON? #3014

Closed
radeksimko opened this issue Aug 17, 2015 · 13 comments
Closed

[proposal] Introduce Schema.TypeJSON? #3014

radeksimko opened this issue Aug 17, 2015 · 13 comments

Comments

@radeksimko
Copy link
Member

In the face of #2589 and similar issues with resources having JSON attributes, I'm thinking of creating schema.TypeJSON which would allow us to do the following:

  1. Have better/standardised/visible way of "normalising" the JSON for state
  2. Expanding from string to struct and flattening from struct to string could work out of the box or with minimal effort
  3. Be able to define default values for structures inside the JSON in a standardised way - typically to avoid situations where user doesn't provide these and API replies with different JSON (having defaults defined)
  4. Be able to set any part of the JSON structure as "computed" - which would work the same way as schema.Schema.Computed = true, but on a field level.

Feedback is welcomed.

@apparentlymart
Copy link
Contributor

Oh heh I just wrote up #3026 and then found this a few minutes later. Sorry for the dupe... this wasn't here when I searched before I started prototyping.

@sparkprime
Copy link
Contributor

It sounds like you did almost exactly what Radek was proposing :)

@radeksimko
Copy link
Member Author

Nice! it doesn't have that logic for nested attributes - setting default values or marking as computed, but that could be part of another PR.

@mitchellh
Copy link
Contributor

See #3026 (my longer form comment) for reasoning on this and why I think we should try something else.

@apparentlymart
Copy link
Contributor

@radeksimko for my use-case the structure genuinely is arbitrary -- chef just considers this to be a bag of stuff that is up to the recipes to interpret.

But for the IAM policy case there is a defined structure to an IAM policy, so maybe it's better to model that structure as a schema.Resource definition and then turn it into JSON as part of the resource implementation, rather than exposing the JSON in the UI. Admittedly the AWS UI itself exposes these in the UI, so that would be overachieving a bit, but it would make it easier to diff and interpolate the structure if Terraform could actually "see" it, rather than considering it just as a plain string.

@radeksimko
Copy link
Member Author

@apparentlymart I'm not quite convinced that turning JSON into DSL is the right approach, as you say AWS UI and many other tools just take the whole JSON document as an atomic unit. It feels like we'd be going against conventions.

@mingfang
Copy link

What was the conclusion of this?
I need to have a TypeJSON field type.

@apparentlymart
Copy link
Contributor

Hi @mingfang!

This is a very old issue and, importantly, both Radek and I were not yet HashiCorp employees during the above discussions even though we are now both working on Terraform full time. With that said...

The forthcoming Terraform v0.12 release includes a new protocol for plugins which allows representation of attribute values in a more precise way. One of the features of this new type system is the ability to declare a particular attribute as being "dynamically typed", which means that a value of any type is expected and it's up to the provider to interpret and validate its type.

However, the plugin SDK will not have support for this immediately at v0.12 launch and so it will not yet be available to provider developers. This is an unfortunate consequence of the need to retain the ability to run new provider releases with Terraform v0.10 and v0.11 for some time after release to allow a graceful upgrade path. In a later SDK release, compatibility with Terraform v0.10 and v0.11 will be dropped and thus the SDK will then be able to support v0.12-specific features like the dynamic type feature.

There is not yet a firm plan on how far in the future that might be because we want to complete the v0.12 release and then see how quickly the community is able to upgrade before deciding now long we should retain support for prior versions in plugins. However, the next phase of discussion about this are likely to begin in the months after the v0.12 final release, whose release cycle started yesterday with the alpha1 release and so final date will depend on what sort of feedback comes from the prerelease builds.

@mingfang
Copy link

mingfang commented Oct 22, 2018

@apparentlymart Thanks for the detailed response.
Unfortunately this sounds like the need for TypeJSON will not get addressed anytime soon, if ever.

This is an example of the problem,
https://github.com/Mastercard/terraform-provider-restapi/blob/5745cb7d4b27b4d7d67de51325668c1bd2221e70/examples/dummy_users_with_fakeserver.tf#L27

In this example, I want to replace this

resource "restapi_object" "Foo" {
  path = "/api/objects"
  data = "{ \"id\": \"1234\", \"first\": \"Foo\", \"last\": \"Bar\" }"
}

with this

resource "restapi_object" "Foo" {
  path = "/api/objects"
  data { 
     id = "1234"
     first = "Foo"
     last = "Bar"
  }
}

This can be done by making the data field type TypeJSON instead of TypeString.

@apparentlymart
Copy link
Contributor

apparentlymart commented Oct 22, 2018

That is the effect of the dynamic-typed attribute mechanism I mentioned; you'd mark data as being dynamically-typed, and then Terraform would skip all type checking for that particular argument and just pass whatever the user gave in to the provider. (How exactly that will appear in the SDK API remains to be designed, though.)

Since it'd be declared as an attribute (rather than a nested resource schema), it'd still be necessary to write it with an equals sign:

resource "restapi_object" "Foo" {
  path = "/api/objects"
  data = { 
     id    = "1234"
     first = "Foo"
     last  = "Bar"
  }
}

In 0.12 the jsonencode function will also be more complete -- supporting all types in the configuration language type system -- so in the mean time users will be able to avoid hand-writing JSON strings using that:

resource "restapi_object" "Foo" {
  path = "/api/objects"
  data_json = jsonencode({ 
     id    = "1234"
     first = "Foo"
     last  = "Bar"
  })
}

Note that an attribute that current takes a JSON string but should be replaced with a dynamically-typed value in future should be named with a _json suffix for now, both to make it clear to users that JSON is what is expected now and to allow for a new un-suffixed name to be introduced alongside it later, during a deprecation cycle for the JSON string attribute.

(restapi_object doesn't do this because it doesn't have any JSON-specific knowledge, and instead just considers the data to upload as an arbitrary string. It's up to the user to decide which serialization format is appropriate, depending on the specification of the API in question.)

@mingfang
Copy link

The dynamic-typed attribute mechanism sounds very promising.
When will it be available?

I'm building a Kubernetes provider https://github.com/mingfang/terraform-provider-k8s
and I need this feature in order to support Kubernetes Custom Resources https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/

For Custom Resources, the "spec" field is dynamic.

@apparentlymart
Copy link
Contributor

As mentioned in my comment a few days ago, Terraform 0.12 will introduce the feature at the protocol level, but offering it at the SDK level is blocked for the moment because the providers will need to continue to support Terraform 0.10 and 0.11 for some period.

Once the Terraform 0.12 release is complete and we see how quickly users are able to adopt it, we will make a decision about when to end support for Terraform 0.10 and 0.11 for new provider releases, which will then allow us to update the SDK to expose this feature. We don't yet have a plan for this because we need to ensure that enough users have been able to upgrade to Terraform 0.12 before requiring it for new provider releases.

In the mean time, the usual approach (as implemented in several other providers) would be to support spec_json for now and then plan to introduce a separate spec field which uses dynamic typing alongside it later on, once the SDK is updated. For now, the documentation examples for the provider should show using the jsonencode function for readability, as I illustrated in my previous comment.

@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants