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

Token System #525

Closed
8 tasks done
skorfmann opened this issue Jan 27, 2021 · 13 comments
Closed
8 tasks done

Token System #525

skorfmann opened this issue Jan 27, 2021 · 13 comments
Assignees
Labels
cdktf enhancement New feature or request feature/tokens priority/important-soon High priority, to be worked on as part of our current release or the following one. size/x-large estimated > 1 month
Milestone

Comments

@skorfmann
Copy link
Contributor

skorfmann commented Jan 27, 2021

We want to address issues around the Token system and make the experience better.

Note: Still have to fill out the details here about what the actual problem is and how we're going to tackle this.

Issues Resolved by this Feature

@skorfmann skorfmann added enhancement New feature or request cdktf labels Jan 27, 2021
@skorfmann skorfmann added this to the 0.2 milestone Jan 27, 2021
@jsteinich
Copy link
Collaborator

One question to answer when tackling this is whether to just fix a few current issues or to prepare for supporting functions / expressions including #220.

I certainly see value in taking a simple approach to improve the immediate experience as I anticipate full support to be a much larger project.

@skorfmann skorfmann mentioned this issue Feb 3, 2021
8 tasks
@jsteinich
Copy link
Collaborator

I've been working on a proposal for how to tackle this. It's not complete and I want to make some pocs, but I want to start writing it out to better kick off discussions.

Current problems

  • Only supports strings, numbers, and lists of strings
  • Doesn't support accessing a single element from a list of strings
  • Confusing output when printing
  • Inconvenient to reference real values

Other desired expansions

  • Support Terraform functions/expressions
  • Provide access to complex computed types

Solution preface

  • Not planning to implement all expansions here, but would like to set up a system which will support them.
  • I think there are some pieces that would only be solvable with Making cdktf dynamic #435 so this will knowingly not solve all issues.

Proposed solution

  • Remove the token system as it currently exists
    • It only works for part of what is needed
    • Not very flexible for desired additions for what it does work with
  • Create a TerraformAttribute class
    • Similar idea to Tokens, but is more explicit about it
    • Allows for keeping track of references
    • Can retrieve raw value if set
    • Can add built-in terraform functions as methods
  • Create subclasses for core types
    • TerraformStringAttribute, TerraformNumberAttribute, TerraformBooleanAttribute, TerraformDynamicAttribute, TerraformListAttribute, TerraformSetAttribute, TerraformMapAttribute, TerraformObjectAttribute
    • This will give the user a more limited set of options for types attributes are
    • Can scope built-in terraform functions that are only designed to work on say collections to appropriate type
    • Possibly do code gen for specific object / collection types to workaround not having generics in jsii
  • Collection types can have additional methods to support accessing elements / iterating in a terraform friendly way
    • The alternative idea of building custom collections into jsii would work well in Java and C# (need to switch to list from array), but I don't believe would work in Typescript (I don't believe it's possible to extend the Array type) (I'm not sure about python)
  • Generate type aliases to allow for using either "primitive" types or TerraformAttributes
    • Still want it to be easy to initialize values
    • Also want to be able to easily reference other resources / data sources
    • Internally initing/setting would always result in a TerraformAttribute (or subtype) being created if not already using one
    • The getter will be a bit deceptive since you'll never get a primitive value from it
    • feat(jsii): support aliasing type unions aws/jsii#1807 is a prerequisite in my mind; else C# will be unusable
    • Provide accessors to nested computed attributes #480 complicates things since the union types will need to match else there will be quite a bit of friction
  • Would be nice to have some language specific libraries to make easier to use
    • One example is having implicit operators in C# to make going between "union type" and "primitive type" easier (partially destructive operation)
  • Still a case where the current token system (or something similar) is necessary
    • When using resources in string interpolation
    • Seems common enough to need support
    • Token would retain reference information to attribute
    • Can make a nicer representation that looks basically like a terraform reference

@skorfmann
Copy link
Contributor Author

That's a great list, thanks for putting this together.

I've been working on a proposal for how to tackle this. It's not complete and I want to make some pocs, but I want to start writing it out to better kick off discussions.

I think even before starting with POCs, it would probably quite helpful to have some pseudo code which maps out the various aspects of the list from a user point of view.

@jsteinich
Copy link
Collaborator

I've started working on some examples / proving my proposed solution here: https://github.com/jsteinich/cdktf-ecs-fargate-tokens
I haven't gotten very far, but I'll continue to update and post here once I have something more meaningful.

@jsteinich
Copy link
Collaborator

  • I've been thinking some more and I can't think of a case where a complex object (class generated for it) is passed from one resource to another, so we shouldn't run into issues with type aliases not matching up.
  • We'll definitely want to do code gen for specific object / collection types since it is relatively common to want to access a particular property in order to pass to another resource.
  • The biggest user facing impacts I see are:
    • When creating resources, properties will have multiple types that they accept. While the simple type one would expect will still work, it's a small amount additional cognitive load
    • We'll no longer be returning arrays (though the signature will make it look possible). Given that it wasn't almost always unsafe to do anything but pass along, I believe this to be a good change; however, will probably not be what uses expect.
    • There are a few other breaking changes, but other than calling them out in release notes, I don't really expect them to cause issues for most users.

@jsteinich
Copy link
Collaborator

When working on adding the list attribute type, I ran into some issues with the returned type:

(publicRouteTable.propagatingVgws as TerraformStringListAttribute).get(0)

since the returned type is TerraformStringList in order to make it match the setter and allow full flexibility.

Some options:

  1. Use TS 4.3 (currently in beta) which allows for having different getter/setter types. Jsii doesn't currently support this and will likely not turn out great for at least c#.
  2. Remove the setter and just return the actual type. Add a set method instead (ala Java) if users want to set a property after the constructor.
  3. Add a differently named getter which always returns the attribute type.

I've ran into this type of situation many times now. I would really like to go with option 2 under the assumption that it is uncommon to set properties after the constructor.

@skorfmann
Copy link
Contributor Author

I think the important parts from a user point of view: It should be idiomatic to use and where possible native types should be usable. Whatever we build, should be composable, e.g. when I deal with a referenced list, it should be compatible with collection functions.

Plus, we do need to track the origin of references to enable automating cross stack references and building up dependencies between stacks.

@jsteinich
Copy link
Collaborator

#376 and #856 have also been linked to this issue.
Don't forget about updating booleans and some other types to take IResolvable.

@mscottnelson
Copy link

User here, just wanted to comment that I ended up on this thread for the following reason:

Exploring terraform-cdk in typescript -> checking out kubernetes provider -> wondering if the interface ServiceSpec.type would ever support a string-literal union type. Also wondering if terraform-cdk will take advantage of typescript tuple-types in the places that it would make sense.

@jsteinich
Copy link
Collaborator

wondering if the interface ServiceSpec.type would ever support a string-literal union type

I'm not sure about string-literal union type, but possibly something along those lines. See #707

will take advantage of typescript tuple-types

Nothing planned here. The upstream jsii library heavily restricts which types are available since they need to map into all the other supported languages.

@DanielMSchmidt DanielMSchmidt added priority/important-soon High priority, to be worked on as part of our current release or the following one. size/x-large estimated > 1 month labels Dec 1, 2021
@DanielMSchmidt
Copy link
Contributor

I'm closing this issue now, we covered everything

@github-actions
Copy link
Contributor

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've 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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cdktf enhancement New feature or request feature/tokens priority/important-soon High priority, to be worked on as part of our current release or the following one. size/x-large estimated > 1 month
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants