Terraform Variables

[jsteinich]

Adds support for defining Terraform Variables #249

[jsteinich]

A couple remaining pieces:

1. Generated names might work alright for future module constructs, but they aren't very useful for configuring the stack. May want to share some details with Forceable Static Name for Outputs #247 though maybe flip the default behavior.
2. Tokens aren't really able to represent all of the variable types. Expanding them is maybe a separate task though.

[jsteinich]

I experimented quite a bit with less verbose ways to define the variable type. Commits 80389e3 and 56a4ebc show a couple iterations on it.
Ultimately I ended up back at my original version with a few tweaks:

• Dropped Terraform from many of the names to make them shorter
• Added 2 ways for shorter collections

1. Static constants for primitive element types
2. An of method that when coupled with static base isn't really shorter, but somehow looks nicer to me

Some of the issues:

• Collections can actually contain any other type, so it isn't possible to enumerate all of them
• I tried a tuple type for defining a collection which looked alright in TS (probably better in TS 4), but completely failed in Java
• I tried defining the tuple type as an array, but at that point it is a recursive type. Recursive types sometimes work, but it caused tsc to spin idefinitely
• I tried object types as an index type which works fine in TS, but is unusable in Java
• Trying to mix enums/interfaces/type aliases made it rather difficult to get the synthesizing logic correct, while it is quite simple with classes

Someone with more Typescript experience might be able to get something cleaner, but with the limitations of jsii and a desire to not make a terrible experience in other languages, I think this is a good balance.

[jsteinich]

I made variables default to using the id as the name. I imagine the common use case being defining root input variables, so that's why I went that route.
I did make it possible to use generated names by setting staticName to false.

[jsteinich]

I think any token expansion should be done as a separate task. I'm not very familiar with how they work under the hood and I'm also not sure how best to represent generic types without generics.

[skorfmann]

limitations of jsii and a desire to not make a terrible experience in other languages, I think this is a good balance.

Could you give an example for the "terrible experience" in other languages which you're trying to avoid here?

[jsteinich]

Could you give an example for the "terrible experience" in other languages which you're trying to avoid here?

export interface ObjectVariableType {
    [key: string]: VariableType;
}

translates to

package org.terrastack;

/**
 * EXPERIMENTAL
 */
@javax.annotation.Generated(value = "jsii-pacmak/1.7.0 (build 179a3a5)", date = "2020-08-10T12:36:14.736Z")
@software.amazon.jsii.Jsii(module = org.terrastack.$Module.class, fqn = "@terrastack/core.ObjectVariableType")
@software.amazon.jsii.Jsii.Proxy(ObjectVariableType.Jsii$Proxy.class)
@software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
public interface ObjectVariableType extends software.amazon.jsii.JsiiSerializable {

    /**
     * @return a {@link Builder} of {@link ObjectVariableType}
     */
    @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
    static Builder builder() {
        return new Builder();
    }
    /**
     * A builder for {@link ObjectVariableType}
     */
    @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
    public static final class Builder implements software.amazon.jsii.Builder<ObjectVariableType> {

        /**
         * Builds the configured instance.
         * @return a new instance of {@link ObjectVariableType}
         * @throws NullPointerException if any required attribute was not provided
         */
        @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
        @Override
        public ObjectVariableType build() {
            return new Jsii$Proxy();
        }
    }

    /**
     * An implementation for {@link ObjectVariableType}
     */
    @software.amazon.jsii.Stability(software.amazon.jsii.Stability.Level.Experimental)
    final class Jsii$Proxy extends software.amazon.jsii.JsiiObject implements ObjectVariableType {

        /**
         * Constructor that initializes the object based on values retrieved from the JsiiObject.
         * @param objRef Reference to the JSII managed object.
         */
        protected Jsii$Proxy(final software.amazon.jsii.JsiiObjectRef objRef) {
            super(objRef);
        }

        /**
         * Constructor that initializes the object based on literal property values passed by the {@link Builder}.
         */
        private Jsii$Proxy() {
            super(software.amazon.jsii.JsiiObject.InitializationMode.JSII);
        }

        @Override
        public com.fasterxml.jackson.databind.JsonNode $jsii$toJson() {
            final com.fasterxml.jackson.databind.ObjectMapper om = software.amazon.jsii.JsiiObjectMapper.INSTANCE;
            final com.fasterxml.jackson.databind.node.ObjectNode data = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();


            final com.fasterxml.jackson.databind.node.ObjectNode struct = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();
            struct.set("fqn", om.valueToTree("@terrastack/core.ObjectVariableType"));
            struct.set("data", data);

            final com.fasterxml.jackson.databind.node.ObjectNode obj = com.fasterxml.jackson.databind.node.JsonNodeFactory.instance.objectNode();
            obj.set("$jsii.struct", struct);

            return obj;
        }
    }
}

[jsteinich]

I poked around a bit more and I may have found a way to get tuples to work as just an array, but I'm still at a loss for how to get objects working as desired in TS while still being usable in Java/C#.

[jsteinich]

I'll probably make some adjustments to the id/name assigned to variables once #329 is resolved, but any other next steps @skorfmann ?

[cmclaughlin]

Would this be a good place to add support for CLI vars?

i.e. like the Terraform CLI has support for -var 'foo=bar' and -var-file=foo

Or perhaps we should open a new issue for that?

[skorfmann]

I'll probably make some adjustments to the id/name assigned to variables once #329 is resolved, but any other next steps @skorfmann ?

I'll have a look at this soon. Gonna try it out locally

[skorfmann]

Would this be a good place to add support for CLI vars?

i.e. like the Terraform CLI has support for -var 'foo=bar' and -var-file=foo

Or perhaps we should open a new issue for that?

I think that should be a new issue 👍

[skorfmann]

My feeling is still, that we shouldn't make it more complicated than the underlying implementation.

Perhaps taking a step back will help us here. What is the actual goal of having typed variable types? Is it just for the sake of auto completion, or are we're actually using them for validation?

Let's take a look at two examples:

1.

// rather than
const imageId = new TerraformVariable(this, 'imageId', {
    type: PrimitiveVariableType.STRING,
    default: 'ami-abcde123',
    description: 'What AMI to use to create an instance'
});

// which is equal to

const imageId = new TerraformVariable(this, 'imageId', {
    type: 'string',
    default: 'ami-abcde123',
    description: 'What AMI to use to create an instance'
});

2.

Or a more complex example to demonstrate my pain points:

// rather than
    new TerraformVariable(stack, 'test-variable', {
        type: new CollectionVariableType(CollectionType.MAP, PrimitiveVariableType.BOOL)
    });

// which is equal to

    new TerraformVariable(stack, 'test-variable', {
        type: "map(bool)"
    });

A user would have to know about and understand how to put together the following types:

• CollectionVariableType
• CollectionType
• PrimitiveVariableType

At that point, I would think it's way easier for the user to supply a plain HCL compatible string. In particular, when considering that Terraform Variables are not a feature which I would expect a common user to use frequently. I think that's more a niche feature for creating modules and migrating a HCL project to cdktf. For that case, it's actually way easier to go with a plain string as well.

Therefore, I'd suggest to drop the typed Variable types and replace it by inline documentation for now (so that it shows up in intellisense.

Anything I missed and / or other thoughts?

[jsteinich]

I think that's more a niche feature

I want to believe this, but I'm not quite convinced it won't be used a bit more than that.

Anything I missed and / or other thoughts?

I hold strongly to not relying on "magic" strings. I pushed what I think is a reasonable compromise though. You can pass the raw string for the hcl type or you can use constants / factory methods on an abstract (static) class to get valid values.

[skorfmann]

I want to believe this, but I'm not quite convinced it won't be used a bit more than that.

Could you provide a few use-cases you have in mind?

[jsteinich]

Could you provide a few use-cases you have in mind?

Mainly thinking about a synth once, deploy multiple workflow.

We currently use https://www.runatlantis.io/docs/custom-workflows.html#use-cases with multiple tfvar files. It would be pretty straightforward to add a synth to every workflow, but I'm also exploring running the synth separately and keeping the atlantis workflow as is.

[skorfmann]

We currently use https://www.runatlantis.io/docs/custom-workflows.html#use-cases with multiple tfvar files. It would be pretty straightforward to add a synth to every workflow, but I'm also exploring running the synth separately and keeping the atlantis workflow as is.

Hm, that's interesting - are you using workspaces as well?

[jsteinich]

Hm, that's interesting - are you using workspaces as well?

Yes, with S3 backend state.

[skorfmann]

Cool, thanks for adding this 👍 Let's merge it and get some real world experience with that :)

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.