feat(provider-generator): handle 'tuple' type for variable #2964

kination · 2023-06-24T04:24:26Z

Related issue

Fixes #1618

Description

Allow tuple for variable type

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

jsteinich

Thanks for taking a shot at this. Need to make a few adjustments and then you should be good to add some test cases.

packages/@cdktf/provider-generator/lib/get/generator/module-generator.ts

kination · 2023-06-28T11:55:14Z

@jsteinich I've make few change, and trying to make test case in https://github.com/hashicorp/terraform-cdk/blob/main/packages/@cdktf/provider-generator/lib/get/__tests__/generator/module-generator.test.ts

(I've put test tf file inside fixtures/local-variable/variable.tf)

test("check valid data types", () => {
  const code = new CodeMaker();
  const module = new TerraformModuleConstraint({
    name: "local_module",
    fqn: "local_module",
    source: path.resolve(__dirname, "fixtures", "local-variable"),
  });
  const target = new ConstructsMakerModuleTarget(module, Language.TYPESCRIPT);
  new ModuleGenerator(code, [target])
...
})

I'm trying with following https://github.com/hashicorp/terraform-cdk/blob/main/packages/%40cdktf/provider-generator/package.json#L15
but test does not goes on...

      at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:427:11)
      at Object.<anonymous> (../hcl2json/wasm/bridge_wasm_exec.js:14:28)
      at ../hcl2json/lib/bridge.ts:39:5
      at init (../hcl2json/lib/bridge.ts:39:5)


 RUNS  lib/get/__tests__/generator/module-generator.test.ts
 RUNS  lib/get/__tests__/read-schema.test.ts

Test Suites: 14 failed, 14 of 16 total
Tests:       14 failed, 40 passed, 54 total
Snapshots:   61 passed, 61 total
Time:        114 s

Could you give suggestion how to make testing for this?

jsteinich · 2023-06-30T01:51:07Z

Could you give suggestion how to make testing for this?

Might just need to reinstall dependencies (yarn install) or deleting a package lock and node_modules directory and trying that.

You can also likely simplify your test to just use expectModuleToMatchSnapshot.

kination · 2023-07-01T01:31:44Z

@jsteinich thanks.
Also, could you give advise how to initialize module generator?
I'm trying as following:
(keeping test tf file inside fixtures/local-variable/variable.tf)

test("check valid data types", () => {
  const code = new CodeMaker();
  const module = new TerraformModuleConstraint({
    name: "local_module",
    fqn: "local_module",
    source: path.resolve(__dirname, "fixtures", "local-variable"),
  });
  const target = new ConstructsMakerModuleTarget(module, Language.TYPESCRIPT);
  new ModuleGenerator(code, [target])
...
})

but it failed as:

...
    missing spec for local_module

      24 |
      25 |     if (!spec) {
    > 26 |       throw new Error(`missing spec for ${target.fqn}`);
         |             ^
      27 |     }
      28 |
      29 |     this.code.openFile(target.fileName);

      at ModuleGenerator.emitSubmodule (lib/get/generator/module-generator.ts:26:13)
      at new ModuleGenerator (lib/get/generator/module-generator.ts:17:12)
      at Object.<anonymous> (lib/get/__tests__/generator/module-generator.test.ts:140:3)

Thanks.

jsteinich · 2023-07-03T23:56:28Z

Also, could you give advise how to initialize module generator?

ModuleGenerator is not something you're likely to ever directly create. The tests have some useful functions to simply module generation. For example:

expectModuleToMatchSnapshot("tuple type", "generator", [
  "local-variable/variable.tf",
]);

kination · 2023-07-08T13:13:40Z

@jsteinich I've just added variable inside existing template.
But it shows following error:

...
   Snapshot name: `getX variables 1`

    - Snapshot  - 1
    + Received  + 1

    @@ -8,11 +8,11 @@
         * @default true
         */
        readonly fetchCallerIdentity?: boolean;
        /**
         * RDS DB subnet group resource
    -    * @default [["172.0.0.1", "172.0.0.2", 1],["172.0.10.1", "172.0.10.2", 2]]
    +    * @default 172.0.0.1,172.0.0.2,1,172.0.10.1,172.0.10.2,2
         */
        readonly fetchSubnetIds?: any[][];
...

What I've expect in comment part should be [["172.0.0.1", "172.0.0.2", 1],["172.0.10.1", "172.0.10.2", 2]], but result shows 172.0.0.1,172.0.0.2,1,172.0.10.1,172.0.10.2,2. Could you confirm this part?

jsteinich · 2023-07-11T01:46:13Z

What I've expect in comment part should be [["172.0.0.1", "172.0.0.2", 1],["172.0.10.1", "172.0.10.2", 2]], but result shows 172.0.0.1,172.0.0.2,1,172.0.10.1,172.0.10.2,2. Could you confirm this part?

That's certainly not what it should be, but I don't believe anything in the PR introduced that behavior. Can make another issue to keep track of that.

kination · 2023-07-11T12:48:45Z

@jsteinich thanks for notice.

Anyway, this part makes test failure, so it couldn't pass the checks. So options I can think is...

Fix value in snap file(https://github.com/hashicorp/terraform-cdk/pull/2964/files#diff-0dd3563bcd9ba2e65e9a8adc75c1133318018e4c80ba1a942def57aa26f113bdR8494) same as generated format, so pass the test.
Fix the generator logic to make result as ["172.0.0.1", "172.0.0.2", 1],["172.0.10.1", "172.0.10.2", 2]]. But this can be out of scope...

How do you think?

jsteinich · 2023-07-12T01:23:03Z

@jsteinich thanks for notice.

Anyway, this part makes test failure, so it couldn't pass the checks. So options I can think is...

Fix value in snap file(https://github.com/hashicorp/terraform-cdk/pull/2964/files#diff-0dd3563bcd9ba2e65e9a8adc75c1133318018e4c80ba1a942def57aa26f113bdR8494) same as generated format, so pass the test.

Fix the generator logic to make result as ["172.0.0.1", "172.0.0.2", 1],["172.0.10.1", "172.0.10.2", 2]]. But this can be out of scope...

How do you think?

I think it makes sense to just update the snapshot so that it passes. Can leave a comment on the test case about it.
@DanielMSchmidt does that sound good to you?

ansgarm · 2023-07-21T14:15:58Z

Updating the snapshot would be fine with me if we also create an issue to improve the generator to fix how these defaults are rendered 👍
@kination @jsteinich

kination · 2023-07-22T02:13:48Z

@ansgarm @jsteinich thanks for suggestion. Than I'll go on with this.

https://github.com/hashicorp/terraform-cdk/actions/runs/5628083685/job/15251540738?pr=2964
=> I'm not sure why this cause failure. It works well when I run it locally. Could you check?

jsteinich · 2023-07-23T21:01:14Z

https://github.com/hashicorp/terraform-cdk/actions/runs/5628083685/job/15251540738?pr=2964
=> I'm not sure why this cause failure. It works well when I run it locally. Could you check?

Looks like this got broken with #2990. You might need to pull in latest main to see the issue locally. Can probably lump it into a follow-up issue to fix.

kination · 2023-07-24T05:02:16Z

@jsteinich could you confirm following once more?
https://github.com/hashicorp/terraform-cdk/actions/runs/5640398393/job/15276937617?pr=2964
Seems template shows same result, but checker indicates as diff ... (some encoding issue?)

jsteinich · 2023-07-24T13:01:46Z

@jsteinich could you confirm following once more? https://github.com/hashicorp/terraform-cdk/actions/runs/5640398393/job/15276937617?pr=2964 Seems template shows same result, but checker indicates as diff ... (some encoding issue?)

I would guess a line ending issue.

kination · 2023-07-24T13:17:01Z

@jsteinich I've tried to change, but git cannot find diff of this.
Could you give me suggestion how to go on with this?

kination · 2023-07-27T09:36:45Z

@jsteinich thanks for update. But seems still there are failures on check. Could you confirm whether these are related with this update?

ansgarm · 2023-07-28T07:58:37Z

Hi @kination 👋

fyi, I just rebased your branch onto main as that should hopefully fix the CI errors you are seeing ✌️

kination · 2023-07-28T08:27:58Z

@ansgarm thanks for your help!
Seems now 1 has left (Vercel authorization). Please let me know if I need to follow-up something for this ... 🤔

ansgarm · 2023-07-28T10:08:52Z

Oh, yeah, missed that. That's just something we have to click for third party PRs 😅

github-actions · 2023-09-04T01:33:05Z

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.

kination requested a review from a team as a code owner June 24, 2023 04:24

kination requested review from mutahhir and DanielMSchmidt and removed request for a team June 24, 2023 04:24

vercel bot deployed to Preview – terraform-cdk-redirects June 24, 2023 04:24 View deployment

team-tf-cdk added ci/provider-integration ci/run-unit/@cdktf/provider-generator labels Jun 24, 2023

kination changed the title ~~chore: read tuple type for variable~~ [WIP] chore: read tuple type for variable Jun 24, 2023

kination mentioned this pull request Jun 24, 2023

Unknown type tuple #1618

Closed

jsteinich requested changes Jun 25, 2023

View reviewed changes

packages/@cdktf/provider-generator/lib/get/generator/module-generator.ts Outdated Show resolved Hide resolved

packages/@cdktf/provider-generator/lib/get/generator/module-generator.ts Outdated Show resolved Hide resolved

kination changed the title ~~[WIP] chore: read tuple type for variable~~ chore: read tuple type for variable Jun 27, 2023

vercel bot deployed to Preview – terraform-cdk-redirects June 28, 2023 11:45 View deployment

vercel bot deployed to Preview – terraform-cdk-redirects July 8, 2023 13:11 View deployment

kination changed the title ~~chore: read tuple type for variable~~ feat(provider-generator): handle 'tuple' type for variable Jul 12, 2023

xiehan assigned ansgarm Jul 20, 2023

vercel bot deployed to Preview – terraform-cdk-redirects July 24, 2023 03:04 View deployment

vercel bot deployed to Preview – terraform-cdk-redirects July 24, 2023 03:12 View deployment

kination and others added 6 commits July 28, 2023 09:57

read tuple type for variable

5cb3d4d

Use valid JSII type only

d9ea314

Add variable inside test case

3ebd402

change snap

dde507a

fix snap

16b080a

fix snapshot line endings

fc904ab

ansgarm force-pushed the fix-1618/indicate-tuple-type branch from 50fd7d1 to fc904ab Compare July 28, 2023 07:58

vercel bot deployed to Preview – terraform-cdk-redirects July 28, 2023 07:58 View deployment

ansgarm requested a review from jsteinich July 28, 2023 10:09

vercel bot deployed to Preview – terraform-cdk July 28, 2023 10:13 View deployment

jsteinich approved these changes Jul 28, 2023

View reviewed changes

DanielMSchmidt added this pull request to the merge queue Aug 4, 2023

Merged via the queue into hashicorp:main with commit 21dcf29 Aug 4, 2023
44 checks passed

mutahhir mentioned this pull request Aug 17, 2023

chore: release 0.18.0 #3088

Merged

kination mentioned this pull request Aug 18, 2023

Display correct data format inside document #3095

Open

1 task

github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(provider-generator): handle 'tuple' type for variable #2964

feat(provider-generator): handle 'tuple' type for variable #2964

kination commented Jun 24, 2023 •

edited

jsteinich left a comment

kination commented Jun 28, 2023

jsteinich commented Jun 30, 2023

kination commented Jul 1, 2023

jsteinich commented Jul 3, 2023

kination commented Jul 8, 2023

jsteinich commented Jul 11, 2023

kination commented Jul 11, 2023

jsteinich commented Jul 12, 2023

ansgarm commented Jul 21, 2023

kination commented Jul 22, 2023

jsteinich commented Jul 23, 2023

kination commented Jul 24, 2023

jsteinich commented Jul 24, 2023

kination commented Jul 24, 2023

kination commented Jul 27, 2023

ansgarm commented Jul 28, 2023

kination commented Jul 28, 2023

ansgarm commented Jul 28, 2023

github-actions bot commented Sep 4, 2023

feat(provider-generator): handle 'tuple' type for variable #2964

feat(provider-generator): handle 'tuple' type for variable #2964

Conversation

kination commented Jun 24, 2023 • edited

Related issue

Description

Checklist

jsteinich left a comment

Choose a reason for hiding this comment

kination commented Jun 28, 2023

jsteinich commented Jun 30, 2023

kination commented Jul 1, 2023

jsteinich commented Jul 3, 2023

kination commented Jul 8, 2023

jsteinich commented Jul 11, 2023

kination commented Jul 11, 2023

jsteinich commented Jul 12, 2023

ansgarm commented Jul 21, 2023

kination commented Jul 22, 2023

jsteinich commented Jul 23, 2023

kination commented Jul 24, 2023

jsteinich commented Jul 24, 2023

kination commented Jul 24, 2023

kination commented Jul 27, 2023

ansgarm commented Jul 28, 2023

kination commented Jul 28, 2023

ansgarm commented Jul 28, 2023

github-actions bot commented Sep 4, 2023

kination commented Jun 24, 2023 •

edited