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

Complex Computed Optional Types #25

Closed
skorfmann opened this issue Apr 26, 2020 · 4 comments · Fixed by #1725
Closed

Complex Computed Optional Types #25

skorfmann opened this issue Apr 26, 2020 · 4 comments · Fixed by #1725
Labels
cdktf enhancement New feature or request priority/important-soon High priority, to be worked on as part of our current release or the following one. size/large estimated < 1 month ux/configuration

Comments

@skorfmann
Copy link
Contributor

There are optional computed complex types in the provider schema, e.g. in the aws_security_group the attributes egress / ingress. Here an excerpt from the schema:

             "egress": {
                "type": [
                  "set",
                  [
                    "object",
                    {
                      // ... attributes 
                    }
                  ]
                ],
                "optional": true,
                "computed": true
              },

As I understand optional && computed, this means there could be egress or ingress rules created by AWS. Which, at least for egress is true. However, apparently the AWS Terraform provider removes this computed object (see comment)

At the moment the attribute is rendered without taking into account the computed option, which looks like this:

  // egress
  private _egress?: SecurityGroupEgress[];
  public get egress() {
    return this._egress; // Getting the computed value is not yet implemented
  }
  public set egress(value: SecurityGroupEgress[]) {
    this._egress = value;
  }

We should probably enable access to the those computed rules as well. Something along the lines of this:

  // egress
  private _egress?: SecurityGroupEgress[];
  public get egress(index?: string) {
    return this._egress || return new SecurityGroupEgressList(this, 'egress', index);
  }
  public set egress(value: SecurityGroupEgress[] | undefined) {
    this._egress = value;
  }

We'd have to generate:

  • an interface (e.g. SecurityGroupEgress) for the optional type
  • a class (e.g. SecurityGroupEgressList) which implements the interface to wrap the computed value
  • Make sure the return type of the getter function is intuitively usable

In particular the last point seems to be difficult.

For now it's probably fine as is, but when heading towards beta we should come back to this.

skorfmann added a commit that referenced this issue Apr 30, 2020
This provides a fix for #12 and includes some refactoring around the resource parsing / emitting.

The primary goal of the refactoring was, to split the parsing from the emitting to make it easier to understand. I'm still not quite happy with the result (in particular around the models, and that some logic is spread across multiple places). I think it needs another iteration, but for alpha it should do.

Right now it's in the "it's working" state, and "jsii" will compile the "AWS" provider without an error. I haven't done a full sanity check of the generated resources, but for the most part it should be usable.

In regards to the complex computed types, I'd see it as a first stab at the problem. It's not flexible and serves a very specific use case only. The goal:

- Make complex computed types accessible
- Provide type information for the computed properties of those types
- Keep it within the constraints of jsii, namely no generics and no proxies (see #12)

A few issues were created as a follow up - see #24 #25 #26 #27 #28 #29 #39
anubhavmishra pushed a commit that referenced this issue May 5, 2020
This provides a fix for #12 and includes some refactoring around the resource parsing / emitting.

The primary goal of the refactoring was, to split the parsing from the emitting to make it easier to understand. I'm still not quite happy with the result (in particular around the models, and that some logic is spread across multiple places). I think it needs another iteration, but for alpha it should do.

Right now it's in the "it's working" state, and "jsii" will compile the "AWS" provider without an error. I haven't done a full sanity check of the generated resources, but for the most part it should be usable.

In regards to the complex computed types, I'd see it as a first stab at the problem. It's not flexible and serves a very specific use case only. The goal:

- Make complex computed types accessible
- Provide type information for the computed properties of those types
- Keep it within the constraints of jsii, namely no generics and no proxies (see #12)

A few issues were created as a follow up - see #24 #25 #26 #27 #28 #29 #39
@joatmon08 joatmon08 added the enhancement New feature or request label May 11, 2020
@joatmon08
Copy link
Contributor

For additional context, see hashicorp/terraform-plugin-sdk/issues/282. May be updated in next version of plugin SDK.

@anubhavmishra
Copy link
Member

Related: #124

@anubhavmishra
Copy link
Member

I will provide a list of providers that can help us prioritize working on this issue.

@anubhavmishra anubhavmishra removed this from the post-launch milestone Jul 22, 2020
@DanielMSchmidt DanielMSchmidt mentioned this issue Aug 19, 2021
8 tasks
@DanielMSchmidt DanielMSchmidt added priority/important-soon High priority, to be worked on as part of our current release or the following one. size/large estimated < 1 month labels Dec 7, 2021
@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 27, 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 priority/important-soon High priority, to be worked on as part of our current release or the following one. size/large estimated < 1 month ux/configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants