feat: supprt cdktf `^0.20.x` #445

BrentSouza · 2024-06-22T00:08:01Z

Fixes #485

Hello. Just figured I would give this a shot! Please let me know if there is any additional work that should be completed. Tests seem to pass when run locally:

 PASS  test/resolve.test.ts (14.014 s)
  √ can resolve direct output value (59 ms)
  √ can resolve indirect output value (9 ms)
  √ can resolve numbers (5 ms)
  √ can resolve booleans (5 ms)
  √ can resolve token maps (5 ms)
  √ cannot resolve literal maps (11 ms)
  √ can resolve token arrays (5 ms)
  √ cannot resolve literal arrays (5 ms)
  √ can resolve expressions (18 ms)
  √ can resolve nested output value (4 ms)
  √ can resolve resource (6 ms)
...
Test Suites: 1 passed, 1 total
Tests:       11 passed, 11 total
Snapshots:   9 updated, 9 total

iliapolo

I'm not opposed to this change but I would like to understand exactly why things don't work without it. Can you share your project setup and the error you are seeing?

Naively, there's nothing preventing peers from using 0.20.7 even now.

iliapolo · 2024-06-26T07:41:11Z

package.json

    "ts-node": "^10.9.2",
    "typescript": "^5.5.2"
  },
  "peerDependencies": {
    "cdk8s": "^2.68.11",
-    "cdktf": "^0.19.1",


This shouldn't be required because the ^ means any minor version above 19 can be used by peers.

@iliapolo I don't believe that is how it works. When a package version has a zero as the major version the caret will not increment the minor version, only the patch.

See this for an example:

https://semver.npmjs.com

Use cdktf and specify ^0.19.0 as the version. You'll see 0.20 package versions are not listed.

@BrentSouza is correct. The notion of patch versions only getting bumped on dependencies below 1.0.0 is npm update docs: https://docs.npmjs.com/cli/v10/commands/npm-update#caret-dependencies-below-100

iliapolo · 2024-06-26T07:42:49Z

package.json

@@ -43,37 +43,37 @@
  },
  "devDependencies": {
    "@cdk8s/projen-common": "0.0.530",
-    "@cdktf/provider-aws": "^17.0.11",


Hmm, is this required by [email protected]?

Yes. The version constraint on the provider package is incompatible with 0.20.7 of the main cdktf package.

CDKTF version 0.20.0 support was added to @cdktf/provider-aws starting @ v19.0.0 which released January 10, 2024. Commit Release

The package.json for the release included typescript ~5.2.0.

iliapolo · 2024-06-26T07:42:57Z

package.json

    "cdktf-cli": "^0.20.7",
    "constructs": "^10.3.0",
    "eslint": "^8",
    "eslint-import-resolver-typescript": "^3.6.1",
    "eslint-plugin-import": "^2.29.1",
    "fs-extra": "^11.2.0",
-    "jest": "^27",


Jest needs to be updated to fix the warning that it has not been tested for typescript 5. Also the conversation in the fix notes indicates that even though some later versions said it worked, it didn't.

kulshekhar/ts-jest#4048

iliapolo · 2024-06-26T07:43:03Z

package.json

    "jest-junit": "^15",
    "jsii": "^5",
    "jsii-diff": "^1.100.0",
-    "jsii-docgen": "^9.2.2",


iliapolo · 2024-06-26T07:43:11Z

package.json

    "jsii-pacmak": "^1.100.0",
    "jsii-rosetta": "^5",
    "projen": "^0.82.8",
    "standard-version": "^9",
-    "ts-jest": "^27",


All of the above version changes came about because I get the following error running npm install:

npm error code ERESOLVE npm error ERESOLVE unable to resolve dependency tree npm error npm error While resolving: [email protected] npm error Found: [email protected] npm error node_modules/typescript npm error typescript@"^5.5.2" from the root project npm error npm error Could not resolve dependency: npm error peer typescript@">=3.8 <5.0" from [email protected] npm error node_modules/ts-jest npm error ts-jest@"^27" from the root project

BrentSouza · 2024-06-26T10:09:48Z

Hi @iliapolo, I should note I'm also using both cdktf and cdk8s in python, which is where the version constraint produces an error for my project.

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
cdk8s-cdktf-resolver 0.0.89 requires cdktf<0.20.0,>=0.19.1, but you have cdktf 0.20.7 which is incompatible.

Granted, the package still installs. I just would like to clear out the errors.

BrentSouza · 2024-06-26T10:13:35Z

For javascript/typescript, here is a basic project setup that will fail:

{
  "name": "test",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC",
  "description": "",
  "dependencies": {
    "@cdk8s/cdktf-resolver": "^0.0.89",
    "cdk8s": "^2.68.82",
    "cdktf": "^0.20.7"
  }
}

Running npm install for this produces:

npm error code ERESOLVE
npm error ERESOLVE unable to resolve dependency tree
npm error
npm error While resolving: [email protected]
npm error Found: [email protected]
npm error node_modules/cdktf
npm error   cdktf@"^0.20.7" from the root project
npm error
npm error Could not resolve dependency:
npm error peer cdktf@"^0.19.1" from @cdk8s/[email protected]
npm error node_modules/@cdk8s/cdktf-resolver
npm error   @cdk8s/cdktf-resolver@"^0.0.89" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.

joekiller

IMO this looks clean and would be appreciated :)

The general theme here is that terraform-cdk is in the v0.20.0 range right now and because of the way npm handles carrot dependencies below 1.0.0 the cdktf and related dependencies need updating.

Side note, the project here itself is always in patch numbers but it can use minor due to NPM's handling of below 1.0.0 carrots too. I think what is expected i.e. that each minor bump isn't compatible with the next in the "zero-dots" (0.X.Y) is expected which would open up the "opportunity" to patch previous versions if needed.

joekiller · 2024-06-28T20:05:06Z

package.json

    "ts-node": "^10.9.2",
    "typescript": "^5.5.2"
  },
  "peerDependencies": {
    "cdk8s": "^2.68.11",
-    "cdktf": "^0.19.1",


@BrentSouza is correct. The notion of patch versions only getting bumped on dependencies below 1.0.0 is npm update docs: https://docs.npmjs.com/cli/v10/commands/npm-update#caret-dependencies-below-100

joekiller · 2024-06-28T20:27:16Z

package.json

@@ -43,37 +43,37 @@
  },
  "devDependencies": {
    "@cdk8s/projen-common": "0.0.530",
-    "@cdktf/provider-aws": "^17.0.11",


CDKTF version 0.20.0 support was added to @cdktf/provider-aws starting @ v19.0.0 which released January 10, 2024. Commit Release

The package.json for the release included typescript ~5.2.0.

joekiller · 2024-06-28T20:29:50Z

package.json

    "@types/fs-extra": "^11.0.4",
-    "@types/jest": "^27",
+    "@types/jest": "^29",


Jest added support for typescript 5.x starting with version 29.1.0 (ts-jest 20.1.0 CHANGELOG.md 2023-03-26)

joekiller · 2024-06-28T20:34:19Z

package.json

    "cdktf-cli": "^0.20.7",
    "constructs": "^10.3.0",
    "eslint": "^8",
    "eslint-import-resolver-typescript": "^3.6.1",
    "eslint-plugin-import": "^2.29.1",
    "fs-extra": "^11.2.0",
-    "jest": "^27",


Jest needs to be updated to fix the warning that it has not been tested for typescript 5. Also the conversation in the fix notes indicates that even though some later versions said it worked, it didn't.

kulshekhar/ts-jest#4048

joekiller · 2024-07-01T20:10:39Z

@BrentSouza a maintenance release (v0.0.91) got pushed out. you might want a merge in main (or rebase) and yarn install to update the lock.

cdk8s-team maintainers

The development dependencies between "cdktf": "^0.19.1" and has already drifted with "@cdktf/provider-aws": "^17.0.11", wanting 0.18. It may be good to consider utilizing npm such that accidental drift isn't happening with these "pre-stable" releases.

Having everyone understand that caret dependencies below 1.0.0 don't float I hope the cdk8s-team will take these PRs more readily as part of their workflow and consider adopting a package manager that fails when peer dependencies fail to sync.

https://semver.org/spec/v2.0.0.html#how-should-i-deal-with-revisions-in-the-0yz-initial-development-phase

Additional comments

The project utilizes yarn and if you tried to npm install the current tree it'll fail too. This difference in results is due to after npm 7 was released the behavior was changed regarding peer dependencies. Before npm would warn about the peer dependencies being incompatible (and this is what yarn does today). npm 7+, released early 2021, will now fail if the peer dependencies are incompatible (probably a good thing!). See npm 7 is now generally available! - peer-dependencies.

In the meantime if you are using a modern npm, you could set the project to use legacy-peer-deps, via npm config set legacy-peer-deps true --location project but this is not idea in the long run.

yarn seems to ignore people reporting "Unmet Peer Dependencies should fail build" regarding this issue and suggests that projects could use a log filter which pretty much ignores the fact that peer dependencies are part of the ecosystem now.

I think this node blog, from 2013(!), Peer Dependencies summarizes the best approach:

One piece of advice: peer dependency requirements, unlike those for regular dependencies, should be lenient. You should not lock your peer dependencies down to specific patch versions. It would be really annoying if one Chai plugin peer-depended on Chai 1.4.1, while another depended on Chai 1.5.0, simply because the authors were lazy and didn't spend the time figuring out the actual minimum version of Chai they are compatible with.

Sadly this project is in the unstable phase so the advice is good but doesn't have as much flexibility as one might desire at this time.

I hope this gets merged.

BrentSouza · 2024-07-03T10:12:08Z

@joekiller Yup I keep rebasing every few days since the automated patches keep introducing conflicts. I’ll push an update this morning.

BrentSouza · 2024-07-03T11:12:41Z

@iliapolo Any additional questions or concerns with the changes outlined in this PR?

joekiller

LGTM!

BrentSouza · 2024-07-16T11:50:39Z

I was away for a bit but I've rebased once again on the latest from main. @iliapolo please let me know if you need any further information or would like to see additional changes.

BrentSouza · 2024-07-24T17:29:33Z

Keeping the fire burning on this one 🔥

Just rebased once again. @iliapolo Hopefully things are well with you. Please let me know if there's anything else I can do for this PR

joekiller · 2024-07-24T17:49:20Z

I too hope Eli is doing well. In the meantime, could anyone else of the @cdk8s-team provide a review here?

iliapolo · 2024-07-28T07:31:38Z

@BrentSouza @joekiller Appreciate the work on this and the calrifications. LGTM 👍

iliapolo · 2024-07-28T07:43:08Z

@BrentSouza apologies but looks like we need to regenerate some snapshot files. Run yarn build and we should be good.

Now that the PR is approved it should just merge once the snapshots are fixed.

Thanks!

BrentSouza · 2024-07-29T12:48:33Z

@iliapolo Running yarn build is showing that it will remove all of the README.md content from API.md. Is there something I should be doing to prevent that?

BrentSouza · 2024-07-29T12:55:09Z

Looks like if I modify the task to use the -r option, the README is included. I can either:

Modify the docgen task in .projen/tasks.json to include the -r option, thus avoiding any diff for API.md
Allow the readme content to be removed from API.md

I'll wait to hear back on what the desired output is.

BrentSouza · 2024-07-31T12:17:13Z

Looking at it a bit more, this is because we're updating jsii-docgen to 10.x.x

Seems like removal of the README content is an intended change for JSII projects since tasks.json is generated as part of the build. I'll rebase to get the latest conflicts resolved and run yarn build then push

Signed-off-by: Brent Souza <[email protected]>

BrentSouza · 2024-07-31T13:21:45Z

@iliapolo Unfortunately I have to bug you once more as the build step requires approval to run.

BrentSouza · 2024-09-05T18:56:16Z

I had to step away from this one for a bit, but I'm happy to rebase and resolve conflicts once more if we can coordinate on the pipeline approval @iliapolo? It's a chore to keep rebasing given the automated updates to the project.

iliapolo · 2024-09-10T07:43:00Z

@BrentSouza yeah I understand. Please rebase and ping me on slack so I can merge before the next rebasing cycle :)

github-actions · 2024-10-10T08:08:14Z

This PR has not received a response in a while and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

joekiller · 2024-10-10T12:32:29Z

@BrentSouza I hope you can rebase and connect with @iliapolo on slack. The link is on the website but here it is for convenience: https://cdk.dev/

BrentSouza force-pushed the cdktf-0.20 branch from 3834bac to deae5d9 Compare June 22, 2024 02:02

BrentSouza changed the title ~~Update deps to support newer cdktf versions~~ chore(deps): Update deps to support newer cdktf versions Jun 22, 2024

BrentSouza force-pushed the cdktf-0.20 branch from deae5d9 to 3b643aa Compare June 24, 2024 23:04

iliapolo reviewed Jun 26, 2024

View reviewed changes

iliapolo assigned BrentSouza Jun 26, 2024

iliapolo mentioned this pull request Jun 26, 2024

cdk8s-cdktf-resolver: Support cdktf ^0.20 #485

Open

2 tasks

BrentSouza force-pushed the cdktf-0.20 branch from 3b643aa to bed622b Compare June 26, 2024 15:29

joekiller reviewed Jun 28, 2024

View reviewed changes

BrentSouza force-pushed the cdktf-0.20 branch from bed622b to 50aef8e Compare July 3, 2024 11:09

joekiller approved these changes Jul 11, 2024

View reviewed changes

BrentSouza force-pushed the cdktf-0.20 branch from 50aef8e to af1ec9e Compare July 16, 2024 11:48

BrentSouza force-pushed the cdktf-0.20 branch from af1ec9e to f8b394c Compare July 24, 2024 17:26

iliapolo approved these changes Jul 28, 2024

View reviewed changes

iliapolo changed the title ~~chore(deps): Update deps to support newer cdktf versions~~ feat(deps): supprt cdktf ^0.20.x Jul 28, 2024

iliapolo changed the title ~~feat(deps): supprt cdktf ^0.20.x~~ feat: supprt cdktf ^0.20.x Jul 28, 2024

brent-at-aam and others added 2 commits July 31, 2024 08:51

Update deps to support newer cdktf versions

124d2ec

Signed-off-by: Brent Souza <[email protected]>

Fix some additional version constraints

ce267c6

Signed-off-by: Brent Souza <[email protected]>

BrentSouza force-pushed the cdktf-0.20 branch from f8b394c to ce267c6 Compare July 31, 2024 13:15

iliapolo added the response-requested Awaiting response from author label Sep 10, 2024

github-actions bot added the closing-soon label Oct 10, 2024

github-actions bot removed closing-soon response-requested Awaiting response from author labels Oct 10, 2024

feat: supprt cdktf ^0.20.x #445

Are you sure you want to change the base?

feat: supprt cdktf ^0.20.x #445

Conversation

BrentSouza commented Jun 22, 2024 • edited Loading

iliapolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrentSouza commented Jun 26, 2024

BrentSouza commented Jun 26, 2024 • edited Loading

joekiller left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joekiller commented Jul 1, 2024

cdk8s-team maintainers

Additional comments

BrentSouza commented Jul 3, 2024

BrentSouza commented Jul 3, 2024

joekiller left a comment

Choose a reason for hiding this comment

BrentSouza commented Jul 16, 2024

BrentSouza commented Jul 24, 2024

joekiller commented Jul 24, 2024 • edited Loading

iliapolo commented Jul 28, 2024

iliapolo commented Jul 28, 2024

BrentSouza commented Jul 29, 2024

BrentSouza commented Jul 29, 2024

BrentSouza commented Jul 31, 2024

BrentSouza commented Jul 31, 2024

BrentSouza commented Sep 5, 2024

iliapolo commented Sep 10, 2024

github-actions bot commented Oct 10, 2024

joekiller commented Oct 10, 2024

feat: supprt cdktf `^0.20.x` #445

feat: supprt cdktf `^0.20.x` #445

BrentSouza commented Jun 22, 2024 •

edited

Loading

iliapolo left a comment •

edited

Loading

BrentSouza commented Jun 26, 2024 •

edited

Loading

joekiller left a comment •

edited

Loading

joekiller commented Jul 24, 2024 •

edited

Loading