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

chore: Enable type coercion again #2750

Merged
merged 9 commits into from
Apr 3, 2023
Merged

Conversation

mutahhir
Copy link
Contributor

@mutahhir mutahhir commented Mar 30, 2023

Related issue

Fixes #

Description

In plain English, describe your approach to addressing the issue linked above. For example, if you made a particular design decision, let us know why you chose this path instead of another solution.

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 team-tf-cdk added size/m and removed size/s labels Mar 30, 2023
@team-tf-cdk team-tf-cdk added size/l and removed size/m labels Mar 31, 2023
@mutahhir mutahhir marked this pull request as ready for review April 3, 2023 08:42
@mutahhir mutahhir requested a review from a team as a code owner April 3, 2023 08:42
@mutahhir mutahhir requested review from ansgarm and Maed223 and removed request for a team April 3, 2023 08:42
nodeIds,
scopedIds
);
// const references = await extractReferencesFromExpression(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These get removed in the iterators PR I believe.

@@ -551,11 +565,128 @@ function convertTFExpressionAstToTs(
return t.stringLiteral("");
}

/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is moved from generation.ts

return "dynamic";
}

export function findExpressionType(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is mostly a copy of the generation.ts one, but doesn't use references, and does more smart things :D

Comment on lines +391 to +404
if (mapping.parameters.length > 0 && mapping.parameters[0].variadic) {
return t.callExpression(callee, [
t.arrayExpression(
argumentExpressions.map((argExpr) =>
coerceType(
scope,
argExpr,
findExpressionType(scope, argExpr),
mapping.parameters[0].type
)
)
),
]);
}
Copy link
Member

Choose a reason for hiding this comment

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

It is also possible that e.g. the second parameter of two is variadic (e.g. cidrsubnets) – which would not be caught by this.

An alternative would be to remove this map and extend the one below to handle the (if any) variadic parameter. I did this in my prototyped code here:

const args: t.Expression[] = [];
mapping.parameters.forEach((param, idx) => {
if (param.variadic) {
// return an array with all remaining children (each mapped accordingly)
args.push(
t.arrayExpression(
tfAst.children
.slice(idx)
.map((child) => terraformThingToTs(child, param.type))
)
);
} else {
const child = tfAst.children[idx];
if (child) {
args.push(terraformThingToTs(child, param.type));
} else if (!param.optional) {
throw new Error(
`Terraform function call to "${name}" is not valid! Parameter at index ${idx} of type ${
param.type
} is not optional but received no value. The following parameters were passed: ${JSON.stringify(
tfAst.children
)}`
);
}
}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in: #2833

@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 May 25, 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.

None yet

4 participants