feat: Exclude packages that has version set by Projen from upgrade task #1665

Hi-Fi · 2022-03-06T19:38:27Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2022-03-07T08:55:02Z

Codecov Report

Merging #1665 (ca136c1) into main (d90284c) will increase coverage by 1.45%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##             main    #1665      +/-   ##
==========================================
+ Coverage   88.06%   89.51%   +1.45%     
==========================================
  Files         132      150      +18     
  Lines        5109     6064     +955     
  Branches     1207     1562     +355     
==========================================
+ Hits         4499     5428     +929     
- Misses        610      634      +24     
- Partials        0        2       +2

Impacted Files	Coverage Δ
src/awscdk/internal.ts	`100.00% <ø> (ø)`
src/cdk/consts.ts	`100.00% <ø> (+36.36%)`	⬆️
src/dev-env.ts	`83.33% <0.00%> (ø)`
src/java/index.ts	`100.00% <ø> (ø)`
src/python/index.ts	`100.00% <ø> (ø)`
src/release/index.ts	`100.00% <ø> (ø)`
src/tasks.ts	`94.11% <ø> (+0.78%)`	⬆️
src/testing.ts	`80.00% <ø> (ø)`
src/textfile.ts	`100.00% <ø> (ø)`
src/toml.ts	`90.90% <ø> (ø)`
... and 179 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f0c7f2...ca136c1. Read the comment docs.

Hi-Fi · 2022-03-07T09:20:59Z

@Chriscbr How does this look related to upgrade issue?

Chriscbr · 2022-03-07T19:39:48Z

.projen/tasks.json

-          "exec": "npm-check-updates --dep dev --upgrade --target=minor --reject='markmac,projen'"
+          "exec": "npm-check-updates --dep dev --upgrade --target=minor --reject='@types/fs-extra,@types/node,@typescript-eslint/eslint-plugin,@typescript-eslint/parser,eslint,jest-junit,npm-check-updates,standard-version,glob,yaml,markmac,projen'"


We should only be rejecting those that are specifically pinned, no? For example here it shows that "@types/fs-extra" is set to "^8" -- so I would think running npm-check-updates with --target=minor on it should still include updates to this dependency.

projen/.projen/deps.json

Lines 7 to 11 in d20bc27

{

"name": "@types/fs-extra",

"version": "^8",

"type": "build"

},

It's more like opinion thing, as with current solution user still gets correct version to lockfile as this step runs the upgrade step.

ncu doesn't seem to be updating versions in that format, so the filtering out just fully pinned (or ones with ~ could be done if needed).

Chriscbr · 2022-03-07T19:48:22Z

test/upgrade-dependencies.test.ts

@@ -192,6 +194,111 @@ test("github runner can be customized", () => {
  expect(upgrade.jobs.pr["runs-on"]).toEqual("self-hosted");
 });

+describe("upgrade task created without projen defined versions", () => {
+  test("at NodeProject", () => {


Let's just test this for NodeProject. I think the resulting change to other project types might be better captured by snapshot tests (like we see in the changes to test/web/__snapshots__/nextjs-project.test.ts.snap)

These tests were also for my TDD approach, but if snapshot tests are the main thing I can remove these.

In this case TDD with snapshots would have been quite a lot of harder and slower.

Chriscbr · 2022-03-07T19:52:21Z

test/upgrade-dependencies.test.ts

+    // Presynthesize upgradeWorkflow to get tasks to project that can be checked
+    prj.upgradeWorkflow?.preSynthesize();
+    const upgradeTask = prj.tasks.tryFind("upgrade");
+    expect(upgradeTask).toBeDefined();
+    if (upgradeTask) {
+      const execSteps = upgradeTask.steps.filter((step) =>
+        step.exec?.toString().includes("npm-check-updates")
+      );
+      execSteps.forEach((execStep) => {
+        expect(execStep.exec).toContain("npm");
+      });
+    }


Let's make an assertion on the snapshot of the project instead, like we have some of the other test cases:

const tasks = synthSnapshot(project)[TaskRuntime.MANIFEST_FILE].tasks; expect(tasks.upgrade.steps...).toStrictEqual(...);

test/upgrade-dependencies.test.ts

Chriscbr · 2022-03-07T20:04:47Z

src/javascript/upgrade-dependencies.ts

+    // Don't add anything if there's only includes that are excluded by e.g. setting the version.
+    if (includeLength === 0 || (includeLength > 0 && ncuIncludesLength > 0)) {


Why do we create the task if includeLength === 0?

Include's are exlicit includes, e.g. projen for upgrade-projen task.

So we should create task if...

we don't have any explicit includes

we have explicit includes, that are not pinned versions (ncuIncludes).

fixes: projen#1659

Closes projen#1480

Hi-Fi force-pushed the feat/upgrade_versions_as_set branch from 541f30b to 50e8645 Compare March 6, 2022 19:39

Hi-Fi force-pushed the feat/upgrade_versions_as_set branch from a46a5ab to fed36c9 Compare March 7, 2022 09:13

Chriscbr suggested changes Mar 7, 2022

View reviewed changes

Hi-Fi added 2 commits March 8, 2022 08:43

feat: Exclude packages that has version set by Projen from upgrade task

c044b82

fixes: projen#1659

Remove include that has exclude

3d57825

Closes projen#1480

Hi-Fi force-pushed the feat/upgrade_versions_as_set branch from 185a24b to 940267b Compare March 8, 2022 06:50

Moved to snapshot testing

ca136c1

Hi-Fi force-pushed the feat/upgrade_versions_as_set branch from 940267b to ca136c1 Compare March 8, 2022 06:57

Chriscbr approved these changes Mar 8, 2022

View reviewed changes

mergify bot merged commit b2625a4 into projen:main Mar 8, 2022

Hi-Fi deleted the feat/upgrade_versions_as_set branch March 8, 2022 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Exclude packages that has version set by Projen from upgrade task #1665

feat: Exclude packages that has version set by Projen from upgrade task #1665

Hi-Fi commented Mar 6, 2022

codecov-commenter commented Mar 7, 2022 •

edited

Loading

Hi-Fi commented Mar 7, 2022

Chriscbr Mar 7, 2022 •

edited

Loading

Hi-Fi Mar 8, 2022

Chriscbr Mar 7, 2022

Hi-Fi Mar 8, 2022

Chriscbr Mar 7, 2022

Chriscbr Mar 7, 2022

Hi-Fi Mar 8, 2022

		"exec": "npm-check-updates --dep dev --upgrade --target=minor --reject='markmac,projen'"
		"exec": "npm-check-updates --dep dev --upgrade --target=minor --reject='@types/fs-extra,@types/node,@typescript-eslint/eslint-plugin,@typescript-eslint/parser,eslint,jest-junit,npm-check-updates,standard-version,glob,yaml,markmac,projen'"

	{
	"name": "@types/fs-extra",
	"version": "^8",
	"type": "build"
	},

		// Don't add anything if there's only includes that are excluded by e.g. setting the version.
		if (includeLength === 0 \|\| (includeLength > 0 && ncuIncludesLength > 0)) {

feat: Exclude packages that has version set by Projen from upgrade task #1665

feat: Exclude packages that has version set by Projen from upgrade task #1665

Conversation

Hi-Fi commented Mar 6, 2022

codecov-commenter commented Mar 7, 2022 • edited Loading

Codecov Report

Hi-Fi commented Mar 7, 2022

Chriscbr Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Hi-Fi Mar 8, 2022

Choose a reason for hiding this comment

Chriscbr Mar 7, 2022

Choose a reason for hiding this comment

Hi-Fi Mar 8, 2022

Choose a reason for hiding this comment

Chriscbr Mar 7, 2022

Choose a reason for hiding this comment

Chriscbr Mar 7, 2022

Choose a reason for hiding this comment

Hi-Fi Mar 8, 2022

Choose a reason for hiding this comment

codecov-commenter commented Mar 7, 2022 •

edited

Loading

Chriscbr Mar 7, 2022 •

edited

Loading