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(cli): implement hcl2cdk #796

Merged
merged 107 commits into from
Jul 26, 2021
Merged

feat(cli): implement hcl2cdk #796

merged 107 commits into from
Jul 26, 2021

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jun 24, 2021

pbpaste | cdktf convert --language=... should print the CDKTF-ified version of what you have in your clipboard

convert-bigger

@DanielMSchmidt DanielMSchmidt changed the title feat(hcl2cdk): implement output transformation feat(cli): implement hcl2cdk Jun 24, 2021
@DanielMSchmidt DanielMSchmidt linked an issue Jun 25, 2021 that may be closed by this pull request
@danieldreier danieldreier added this to the 0.5 milestone Jun 25, 2021
@danieldreier danieldreier added committed enhancement New feature or request labels Jun 25, 2021
@DanielMSchmidt DanielMSchmidt force-pushed the tf-to-cdk branch 8 times, most recently from 13c6f58 to 8e0b43e Compare July 5, 2021 19:55
@DanielMSchmidt DanielMSchmidt force-pushed the tf-to-cdk branch 4 times, most recently from d1dbf7b to b1e1e05 Compare July 7, 2021 17:08
Copy link
Collaborator

@jsteinich jsteinich left a comment

Choose a reason for hiding this comment

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

I'd be happy to try this out on some of our Terraform projects once it is at that point. Just let me know.

packages/cdktf-cli/bin/cmds/convert.ts Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/schema.ts Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/schema.ts Show resolved Hide resolved
const props = item[0];

if (props.alias) {
throw new Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be hard to add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just wanted to be explicit about what is currently supported :)

@DanielMSchmidt DanielMSchmidt force-pushed the tf-to-cdk branch 3 times, most recently from ca82f9e to d86b877 Compare July 13, 2021 12:10
@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review July 13, 2021 17:09
Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

That's pretty amazing, looks great! Built and tried it locally as well, seems to work as expected in general. For complex examples, there are a few issues though:

resource "aws_acm_certificate" "example" {
  domain_name       = "example.com"
  validation_method = "DNS"
}

data "aws_route53_zone" "example" {
  name         = "example.com"
  private_zone = false
}

resource "aws_route53_record" "example" {
  for_each = {
    for dvo in aws_acm_certificate.example.domain_validation_options : dvo.domain_name => {
      name   = dvo.resource_record_name
      record = dvo.resource_record_value
      type   = dvo.resource_record_type
    }
  }

  allow_overwrite = true
  name            = each.value.name
  records         = [each.value.record]
  ttl             = 60
  type            = each.value.type
  zone_id         = data.aws_route53_zone.example.zone_id
}

resource "aws_acm_certificate_validation" "example" {
  certificate_arn         = aws_acm_certificate.example.arn
  validation_record_fqdns = [for record in aws_route53_record.example : record.fqdn]
}

resource "aws_lb_listener" "example" {
  # ... other configuration ...

  certificate_arn = aws_acm_certificate_validation.example.certificate_arn
}

produces

const awsAcmCertificateExample = new aws.AcmCertificate(this, "example", {
  domainName: "example.com",
  validationMethod: "DNS",
});
const dataAwsRoute53Zone = new aws.Route53Zone(this, "example", {
  name: "example.com",
  privateZone: false,
});
const awsRoute53RecordExample = new aws.Route53Record(this, "example", {
  allowOverwrite: true,
  name: "${each.value.name}",
  records: ["${each.value.record}"],
  ttl: 60,
  type: "${each.value.type}",
  zoneId: dataAwsRoute53Zone.example.zoneId,
});
awsRoute53RecordAwsRoute53RecordExample.addOverride(
  "for_each",
  `\${{
    for dvo in ${awsAcmCertificateExample.domainValidationOptions} : dvo.domain_name => {
      name   = dvo.resource_record_name
      record = dvo.resource_record_value
      type   = dvo.resource_record_type
    }
  }}`
);
const awsAcmCertificateValidationExample = new aws.AcmCertificateValidation(
  this,
  "example",
  {
    certificateArn: awsAcmCertificateExample.arn,
    validationRecordFqdns: `\${[for record in ${awsRoute53RecordExample.fqn} : record.fqdn]}`,
  }
);
new aws.LbListener(this, "example", {
  certificateArn: awsAcmCertificateValidationExample.certificateArn,
});

The thing which doesn't fit there:

const awsRoute53RecordExample = new aws.Route53Record(this, "example", {
  allowOverwrite: true,
  name: "${each.value.name}",
  records: ["${each.value.record}"],
  ttl: 60,
  type: "${each.value.type}",
  zoneId: dataAwsRoute53Zone.example.zoneId,
});
awsRoute53RecordAwsRoute53RecordExample.addOverride(
  "for_each",
  `\${{
    for dvo in ${awsAcmCertificateExample.domainValidationOptions} : dvo.domain_name => {
      name   = dvo.resource_record_name
      record = dvo.resource_record_value
      type   = dvo.resource_record_type
    }
  }}`
);

Also, the imports attribute seems to be empty in this case. I'd have expected that the AWS provider would have been returned for this example.

@DanielMSchmidt
Copy link
Contributor Author

Also, the imports attribute seems to be empty in this case. I'd have expected that the AWS provider would have been returned for this example.

Currently I find the provider from the provider config. The imports fields goal is more in the import complete project part, the code part is for on the spot conversion :) Does this make sense?

The thing which doesn't fit there:

Sorry I'm blind to the error, could you push a failing snapshot test for me to fix? :D

@skorfmann
Copy link
Contributor

The override combines resource class name with the variable name: awsRoute53RecordAwsRoute53RecordExample => Route53Record + awsRoute53RecordExample

@DanielMSchmidt
Copy link
Contributor Author

@skorfmann Ah thank you! Fixed that one :)

@DanielMSchmidt DanielMSchmidt force-pushed the tf-to-cdk branch 3 times, most recently from f22c21a to 5f37289 Compare July 19, 2021 16:36
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.

Did not quite make it through yet, but here is a first set of comments already 👍
I really like the new commands you added to the code after our video review 💪

docs/cli-commands.md Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/README.md Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/README.md Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/README.md Show resolved Hide resolved
packages/@cdktf/hcl2cdk/README.md Outdated Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/helper/init.ts Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/index.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Show resolved Hide resolved
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.

Nice work 💱

@DanielMSchmidt DanielMSchmidt merged commit 1389f7b into main Jul 26, 2021
@DanielMSchmidt DanielMSchmidt deleted the tf-to-cdk branch July 26, 2021 13:29
@jsteinich
Copy link
Collaborator

thanks for taking the time to review

I didn't actually review the code, just started trying it out on some real Terraform projects.

I tried it again this morning and ran into some new issues:

  • In a project without any providers, it still tries to run cdktf get, which fails. Probably an unusual case right now, but if we update to use pre-built providers, then will likely be common.
  • The git modules failed with cdktf get. Not directly related to this. More of an error with the module auto naming, Could probably generate cdktf.json with the name override matching what it was called in hcl.

@DanielMSchmidt
Copy link
Contributor Author

In a project without any providers, it still tries to run cdktf get, which fails. Probably an unusual case right now, but if we update to use pre-built providers, then will likely be common.

Oh great point, I wonder if we should change this behaviour in get in general 🤔 but for now I can add an extra check 👍

The git modules failed with cdktf get. Not directly related to this. More of an error with the module auto naming, Could probably generate cdktf.json with the name override matching what it was called in hcl.

Did you try the latest version? I changed the behaviour to use TerraformHclModule for everything besides registry modules

@jsteinich
Copy link
Collaborator

I wonder if we should change this behaviour in get in general

I think so, but I was viewing that as a follow up task.

Did you try the latest version? I changed the behaviour to use TerraformHclModule for everything besides registry modules

Yep. The code is updated, but it is still present in cdktf.json

@DanielMSchmidt
Copy link
Contributor Author

Yep. The code is updated, but it is still present in cdktf.json

That is really weird, I'm using the same check for both 🤔

.filter(([{ source }]) => isRegistryModule(source))

@jsteinich
Copy link
Collaborator

That is really weird, I'm using the same check for both

I'm thinking it's still in moduleRequirements.

@DanielMSchmidt
Copy link
Contributor Author

Oh right, thank you! Forgot the third part, will do a PR :)

@github-actions
Copy link
Contributor

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 Dec 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HCL to CDKTF Tool
5 participants