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

fix(hcl2cdk): handle JSII Rosetta translation properly #2706

Merged
merged 11 commits into from
Mar 13, 2023

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Mar 9, 2023

Related issue

Fixes #2646

Description

JSII Rosetta needs to have compilable code in order to determine the correct classes to use in Non-TS languages.
This PRs aims to do this for cases where we have node.js bindings in the same folder this runs in. This is the step on the way of making it work in all cases

Follow up work

I decided against doing a full test setup here that synthesises across all languages to cut scope from this PR, I'd suggest we do this in separate follow up PRs.

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

@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.

@DanielMSchmidt DanielMSchmidt marked this pull request as ready for review March 9, 2023 15:50
@DanielMSchmidt DanielMSchmidt requested review from ansgarm and Maed223 and removed request for a team March 9, 2023 15:50
Signed-off-by: hashicorp-copywrite[bot] <110428419+hashicorp-copywrite[bot]@users.noreply.github.com>
@DanielMSchmidt DanielMSchmidt requested review from mutahhir and removed request for Maed223 March 9, 2023 15:50
@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.

@DanielMSchmidt
Copy link
Contributor Author

This should also have fixed #2647 but the snapshots beg to differ for some languages, I'll double check this with the test repository to see if the output is the same. For now we will consider #2647 a separate issue

@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

@mutahhir mutahhir left a comment

Choose a reason for hiding this comment

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

Nice change 👏🏽

@DanielMSchmidt DanielMSchmidt merged commit 21c14fc into main Mar 13, 2023
@DanielMSchmidt DanielMSchmidt deleted the jsii-rosetta-struct-fixes branch March 13, 2023 09:35
@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 13, 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.

Use correct types instead of POD classes in strongly typed languages
3 participants