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.

Sign up for GitHub

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

Jump to bottom

chore: Use new Terraform Expression AST for generating Typescript expression #2744

Merged
merged 25 commits into from
Mar 30, 2023
Merged

chore: Use new Terraform Expression AST for generating Typescript expression #2744

merged 25 commits into from
Mar 30, 2023

Conversation

mutahhir
Copy link
Contributor

@mutahhir mutahhir commented Mar 27, 2023 •
edited

Related issue

Description

This replaces the 'find reference and replace reference' logic with a Terraform Expression AST -> TS AST converter. This is definitely not tested enough, and we will encounter issues. While this approach is overall better, the big caveat is that we're actively re-creating the expressions instead of doing precision string replacements. This requires that the converter is resilient and the edge cases are handled well.

WIP

While this is ready to merge as a block of work in this direction, it's by no means complete. Things that we need to address in future PRs:

  • Better functions support Convert: Use built-in functions #1010
  • Re-add iterators support (added to board)
  • Adjust coercion algorithm to be more integrated to this process and not use references (added to board)
  • Update any documentation changes (added to board)
  • Find and fix more edge cases

Checklist

  • I have updated the PR title to match CDKTF's style guide
  • I have run the linter on my code locally
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation if applicable
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works if applicable
  • New and existing unit tests pass locally with my changes

DanielMSchmidt
DanielMSchmidt reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

Excited for this 🚀

packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
packages/@cdktf/hcl2cdk/lib/expressions.ts Outdated Show resolved Hide resolved
@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.

@mutahhir mutahhir marked this pull request as ready for review March 29, 2023 08:39
@mutahhir mutahhir requested a review from a team as a code owner March 29, 2023 08:39
@mutahhir mutahhir requested review from DanielMSchmidt and Maed223 and removed request for a team March 29, 2023 08:39
@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.

@mutahhir mutahhir enabled auto-merge March 30, 2023 10:45
@mutahhir mutahhir merged commit 1df4814 into main Mar 30, 2023
@mutahhir mutahhir deleted the chore/expression-to-ts branch March 30, 2023 11:34
@mutahhir mutahhir mentioned this pull request Apr 3, 2023
Closed
@mutahhir mutahhir mentioned this pull request Apr 18, 2023
Merged
@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 Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DanielMSchmidt DanielMSchmidt DanielMSchmidt approved these changes

@Maed223 Maed223 Awaiting requested review from Maed223 Maed223 is a code owner automatically assigned from hashicorp/cdktf

Assignees
No one assigned
Labels
ci/run-unit/@cdktf/hcl2cdk ci/run-unit/@cdktf/hcl2json size/xl
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mutahhir @team-tf-cdk @DanielMSchmidt