Install with frozen-lockfile in routerlicious's docker build #23558

Abe27342 · 2025-01-15T01:23:10Z

Description

Adjusts routerlicious's docker build to allow it to install with a frozen lockfile.

See #23535 for some additional context.

Copilot reviewed 4 out of 23 changed files in this pull request and generated no comments.

Files not reviewed (19)

server/routerlicious/Dockerfile: Language not supported
server/routerlicious/package.json: Language not supported
server/routerlicious/packages/gitresources/package.json: Language not supported
server/routerlicious/packages/kafka-orderer/package.json: Language not supported
server/routerlicious/packages/lambdas-driver/package.json: Language not supported
server/routerlicious/packages/lambdas/package.json: Language not supported
server/routerlicious/packages/local-server/package.json: Language not supported
server/routerlicious/packages/memory-orderer/package.json: Language not supported
server/routerlicious/packages/protocol-base/package.json: Language not supported
server/routerlicious/packages/routerlicious-base/package.json: Language not supported
server/routerlicious/packages/routerlicious/package.json: Language not supported
server/routerlicious/packages/services-client/package.json: Language not supported
server/routerlicious/packages/services-core/package.json: Language not supported
server/routerlicious/packages/services-ordering-kafkanode/package.json: Language not supported
server/routerlicious/packages/services-ordering-rdkafka/package.json: Language not supported
server/routerlicious/packages/services-ordering-zookeeper/package.json: Language not supported
server/routerlicious/packages/services-shared/package.json: Language not supported
server/routerlicious/packages/services-telemetry/package.json: Language not supported
server/routerlicious/packages/services-utils/package.json: Language not supported

Abe27342 · 2025-01-15T19:34:23Z

the dep/typetest updates here get kind of involved. I'm open to splitting those to a precursor PR if reviewer would like.

Abe27342 · 2025-01-15T19:36:23Z

server/routerlicious/packages/routerlicious/package.json

@@ -83,6 +83,8 @@
 		}
 	},
 	"typeValidation": {
-		"broken": {}
+		"broken": {},
+		"disabled": true,


This package has a bogus main and types entry: it has no actual index.ts. Typetests using the previous build setup happened to be OK with this, but flub is not. Thus I have disabled them since the package has no API surface anyway.

It seems like if we want to publish this package, we should probably be exposing a script under bin or something to run routerlicious, but going to consider that out of scope for this change...

Agreed. Maybe a (type-tests) entrypoint for each lambda would be an alternative, but with this being just a reference implementation, I'm not sure even that is necessary.

Abe27342 · 2025-01-15T22:19:39Z

server/routerlicious/package.json

@@ -55,7 +56,7 @@
 		"stop": "docker compose down",
 		"stop:full": "docker compose down -v",
 		"test": "pnpm run -r --no-sort --stream --no-bail test",
-		"test:copyresults": "copyfiles --exclude \"**/node_modules/**\" \"**/nyc/**\" nyc",
+		"test:copyresults": "copyfiles --exclude \"**/node_modules/**\" \"packages/**/nyc/**\" nyc",


CI failed with strange issues here, and when I was debugging copyresults locally I realized that the input glob here can match files written to output. Not sure exactly how that manifests in CI not finding results files, but given copyfiles looks to be highly parallelized, it seemed worth a shot to just make this change.

This didn't fix the issue, but on further investigation I also noticed that we were reporting coverage on everything in the server directory including node_module dependencies. Adding some reasonable configurations that resemble the package root seems to have fixed this in latest build (hopefully it's not a flaky thing that reoccurs).

Hitting a limit like this while changing deps makes some vague amount of sense; I believe the newer version of flub has a heavier footprint. I do know at some point I fixed an issue with coverage caused by OOM issues merging reports when not using the merge-async option, so it seems plausible to me this pushed something in the report writing over the edge on CI machines.

Any ideas why there would be any nyc directories (or src/lib/dist ones) outside packages/ ? 🤔

alexvy86

The docker image reference needs to be fixed, the rest looks good.

After this is merged, I'll spend a bit of time deploying the new docker images to our internal cluster so e2e tests can validate that everything looks correct there.

alexvy86 · 2025-01-16T23:04:55Z

server/routerlicious/Dockerfile

@@ -5,7 +5,22 @@

 # Use a direct sha256 hash to ensure we are always using the same version of the base image.
 # This avoids throttling issues with Docker Hub by using an image baked into the pipeline build image.
-FROM node:18.20.5-bookworm-slim@sha256:c4a278056a0f79abf472d912bbf5af6c874126ae450faad9df52069e9ad78335 AS base
+FROM node:18.17.1-buster-slim@sha256:cb2a746612c2564d3bd0f871174618337af9d0f4d895df6a623fd7a69ca6e5bd AS runnerbase


Suggested change

FROM node:18.17.1-buster-slim@sha256:cb2a746612c2564d3bd0f871174618337af9d0f4d895df6a623fd7a69ca6e5bd AS runnerbase

FROM node:18.20.5-bookworm-slim@sha256:c4a278056a0f79abf472d912bbf5af6c874126ae450faad9df52069e9ad78335 AS runnerbase

alexvy86 · 2025-01-16T23:10:44Z

server/routerlicious/package.json

@@ -55,7 +56,7 @@
 		"stop": "docker compose down",
 		"stop:full": "docker compose down -v",
 		"test": "pnpm run -r --no-sort --stream --no-bail test",
-		"test:copyresults": "copyfiles --exclude \"**/node_modules/**\" \"**/nyc/**\" nyc",
+		"test:copyresults": "copyfiles --exclude \"**/node_modules/**\" \"packages/**/nyc/**\" nyc",


Any ideas why there would be any nyc directories (or src/lib/dist ones) outside packages/ ? 🤔

alexvy86 · 2025-01-16T23:26:09Z

server/routerlicious/packages/routerlicious/package.json

@@ -83,6 +83,8 @@
 		}
 	},
 	"typeValidation": {
-		"broken": {}
+		"broken": {},
+		"disabled": true,


Agreed. Maybe a (type-tests) entrypoint for each lambda would be an alternative, but with this being just a reference implementation, I'm not sure even that is necessary.

alexvy86 · 2025-01-16T23:32:31Z

Oh, also, a nice follow-up item at some point would be to use the same pnpm overrides we use in the root of the repo, in all the server pnpm workspaces, to get rid of all the aws dependencies that oclif brings in but we don't use.

Install with frozen-lockfile in routerlicious's docker build

1c35c10

Copilot bot review requested due to automatic review settings January 15, 2025 01:23

Copilot AI reviewed Jan 15, 2025

View reviewed changes

github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Jan 15, 2025

Fix yaml

093197b

github-actions bot added the area: build Build related issues label Jan 15, 2025

Update typetest scripts

33570cf

Abe27342 commented Jan 15, 2025

View reviewed changes

Abram Sanderson added 2 commits January 15, 2025 11:56

fix naming of server release group

0533d55

Tweak copyfiles glob

a5e956f

Abe27342 commented Jan 15, 2025

View reviewed changes

alexvy86 mentioned this pull request Jan 15, 2025

Update docker files to use Debian bookworm #23543

Merged

2 tasks

Abram Sanderson and others added 3 commits January 15, 2025 17:14

Add basic include/exclude patterns to routerlicious c8

857d08c

Merge branch 'main' into test/absander/frozen-lockfile-routerlicious

0eead62

fix formatting, update lockfile

bd24f52

alexvy86 approved these changes Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install with frozen-lockfile in routerlicious's docker build #23558

Install with frozen-lockfile in routerlicious's docker build #23558

Abe27342 commented Jan 15, 2025

Abe27342 commented Jan 15, 2025

Abe27342 Jan 15, 2025

alexvy86 Jan 16, 2025

Abe27342 Jan 15, 2025 •

edited

Loading

Abe27342 Jan 16, 2025

alexvy86 Jan 16, 2025

alexvy86 left a comment

alexvy86 Jan 16, 2025

alexvy86 Jan 16, 2025

alexvy86 Jan 16, 2025

alexvy86 commented Jan 16, 2025 •

edited

Loading

	FROM node:18.17.1-buster-slim@sha256:cb2a746612c2564d3bd0f871174618337af9d0f4d895df6a623fd7a69ca6e5bd AS runnerbase
	FROM node:18.20.5-bookworm-slim@sha256:c4a278056a0f79abf472d912bbf5af6c874126ae450faad9df52069e9ad78335 AS runnerbase

Install with frozen-lockfile in routerlicious's docker build #23558

Are you sure you want to change the base?

Install with frozen-lockfile in routerlicious's docker build #23558

Conversation

Abe27342 commented Jan 15, 2025

Description

Choose a reason for hiding this comment

Abe27342 commented Jan 15, 2025

Abe27342 Jan 15, 2025

Choose a reason for hiding this comment

alexvy86 Jan 16, 2025

Choose a reason for hiding this comment

Abe27342 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Abe27342 Jan 16, 2025

Choose a reason for hiding this comment

alexvy86 Jan 16, 2025

Choose a reason for hiding this comment

alexvy86 left a comment

Choose a reason for hiding this comment

alexvy86 Jan 16, 2025

Choose a reason for hiding this comment

alexvy86 Jan 16, 2025

Choose a reason for hiding this comment

alexvy86 Jan 16, 2025

Choose a reason for hiding this comment

alexvy86 commented Jan 16, 2025 • edited Loading

Abe27342 Jan 15, 2025 •

edited

Loading

alexvy86 commented Jan 16, 2025 •

edited

Loading