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

feat: allow partial snippet translation #2920

Merged
merged 23 commits into from
Jul 5, 2023

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jun 6, 2023

Required attributes not contained in the original HCL will be passed in as options into the constructor. Instead of failing these examples at synth time (leading to another set of issues with JSII-Rosetta not being able to use the correct structs) we deal with these issues ahead of time by adding the required info.

This is a simple example to illustrate:

resource "docker_config" "service_config" {
        name = "my-config"
        # other required properties are missing:
        # data = base64encode(...)
}

This gets converted to Typescript as

import * as constructs from "constructs";
import * as cdktf from "cdktf";
/*Provider bindings are generated by running cdktf get.
See https://cdk.tf/provider-generation for more details.*/
import * as docker from "./.gen/providers/docker";
interface MyConfig {
  data: any;
}
class MyConvertedCode extends cdktf.TerraformStack {
  constructor(scope: constructs.Construct, name: string, config: MyConfig) {
    super(scope, name);
    new docker.config.Config(this, "service_config", {
      name: "my-config",
      data: config.data,
    });
  }
}

JSII processes it into this Java code:

import software.constructs.*;
import com.hashicorp.cdktf.*;
/*Provider bindings are generated by running cdktf get.
See https://cdk.tf/provider-generation for more details.*/
import gen.providers.docker.config.*;
public class MyConfig {
    private Object data;
    public Object getData() {
        return this.data;
    }
    public MyConfig data(Object data) {
        this.data = data;
        return this;
    }
}
public class MyConvertedCode extends TerraformStack {
    public MyConvertedCode(Construct scope, String name, MyConfig config) {
        super(scope, name);
        new Config(this, "service_config", new ConfigConfig()
                .name("my-config")
                .data(config.getData())
                );
    }
}

And e.g. in Golang its

import constructs "github.com/aws/constructs-go/constructs"
import cdktf "github.com/hashicorp/terraform-cdk-go/cdktf"
/*Provider bindings are generated by running cdktf get.
See https://cdk.tf/provider-generation for more details.*/
import "github.com/aws-samples/dummy/gen/providers/docker/config"
type myConfig struct {
	data interface{}
}
type myConvertedCode struct {
	terraformStack
}

func newMyConvertedCode(scope construct, name *string, config myConfig) *myConvertedCode {
	this := &myConvertedCode{}
	cdktf.NewTerraformStack_Override(this, scope, name)
	config.NewConfig(this, jsii.String("service_config"), &configConfig{
		name: jsii.String("my-config"),
		data: config.data,
	})
	return this
}

TODOs

  • We don't share the "isRequired" information with the @cdktf/provider-generator and it could be hard to keep in sync since the information is buried in a parser
  • Only resources and data sources use this in the main part of their config
  • Only tested with top-level attributes, needs testing with block types and nested attributes as well
  • The any type is used instead of (in the example above docker.config.ConfigConfig["data"]), we would need to test if this improves JSIIs capabilities or makes it worse for some reason

fixes: #2822

@team-tf-cdk
Copy link
Collaborator

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@team-tf-cdk
Copy link
Collaborator

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@team-tf-cdk
Copy link
Collaborator

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@team-tf-cdk
Copy link
Collaborator

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me! Just some comments / questions / leftover todos

packages/@cdktf/hcl2cdk/lib/types.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/partialCode.ts Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/partialCode.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/generation.ts Show resolved Hide resolved
@DanielMSchmidt DanielMSchmidt added this pull request to the merge queue Jul 5, 2023
Merged via the queue into main with commit fca12a8 Jul 5, 2023
20 checks passed
@DanielMSchmidt DanielMSchmidt deleted the spike-partial-hcl-translation branch July 5, 2023 08:02
@mutahhir mutahhir mentioned this pull request Jul 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add convert option to treat all missing vars as inputs to the Construct / Stack
4 participants