Improve Task contructor typings

This enforces that the context in the functions defined when
constructing a task are treated as Read-Only objects. This prevents a
task from modifying part of the state outside what is referenced by its
defined lens path.

Change-type: minor

lib/task/props.ts

@@ -12,6 +12,12 @@ export type ContextWithSystem<
 	TOp extends AnyOp = Update,
 > = Context<TState, TPath, TOp> & { system: TState };
 
+type ReadOnlyContextWithSystem<
+	TState = unknown,
+	TPath extends PathType = Root,
+	TOp extends AnyOp = Update,
+> = ReadOnly<Context<TState, TPath, TOp> & { system: TState }>;
+
 /**
  * A descriptor for this task. The descriptor can be either a string or a Context
  * instance. The description does not receive the current state to allow actions to
@@ -29,7 +35,7 @@ type ConditionFn<
 	TOp extends AnyOp = Update,
 > = (
 	s: TOp extends Create ? never : ReadOnly<Lens<TState, TPath>>,
-	c: ReadOnly<ContextWithSystem<TState, TPath, TOp>>,
+	c: ReadOnlyContextWithSystem<TState, TPath, TOp>,
 ) => boolean;
 
 type EffectFn<
@@ -38,7 +44,7 @@ type EffectFn<
 	TOp extends AnyOp = Update,
 > = (
 	view: View<TState, TPath, TOp>,
-	ctx: ContextWithSystem<TState, TPath, TOp>,
+	ctx: ReadOnlyContextWithSystem<TState, TPath, TOp>,
 ) => void;
 
 type ActionFn<
@@ -47,7 +53,7 @@ type ActionFn<
 	TOp extends AnyOp = Update,
 > = (
 	view: View<TState, TPath, TOp>,
-	ctx: ContextWithSystem<TState, TPath, TOp>,
+	ctx: ReadOnlyContextWithSystem<TState, TPath, TOp>,
 ) => Promise<void>;
 
 type MethodFn<
@@ -56,7 +62,7 @@ type MethodFn<
 	TOp extends AnyOp = Update,
 > = (
 	s: TOp extends Create ? never : ReadOnly<Lens<TState, TPath>>,
-	c: ReadOnly<ContextWithSystem<TState, TPath, TOp>>,
+	ctx: ReadOnlyContextWithSystem<TState, TPath, TOp>,
 ) => Instruction<TState> | Array<Instruction<TState>>;
 
 /**

tests/composer/tasks.ts

@@ -36,15 +36,12 @@ export const fetchImage = App.task({
 	},
 	action: async (image, { imageName }) => {
 		await new Promise((resolve, reject) =>
-			docker
-				.pull(imageName)
-				.catch(reject)
-				.then((stream) => {
-					stream.on('data', () => void 0);
-					stream.on('error', reject);
-					stream.on('close', resolve);
-					stream.on('finish', resolve);
-				}),
+			docker.pull(imageName).then((stream) => {
+				stream.on('data', () => void 0);
+				stream.on('error', reject);
+				stream.on('close', resolve);
+				stream.on('finish', resolve);
+			}),
 		);
 
 		// Get the image using the name

tests/orchestrator/planner.ts

@@ -10,7 +10,7 @@ import {
 	migrateService,
 	startService,
 	stopService,
-	removeService,
+	uninstallService,
 	removeRelease,
 	removeApp,
 } from './tasks';
@@ -20,15 +20,11 @@ export const planner = Planner.from<Device>({
 		fetch,
 		createApp,
 		createRelease,
-		// NOTE: right now we need to make sure to put `migrateService` before
-		// `installService` in the task list, otherwise the planner will always chose to
-		// recreate services even if a migration suffices. This is because the planner
-		// just returns the first path it finds instead of the shortest
 		migrateService,
 		installService,
 		startService,
 		stopService,
-		removeService,
+		uninstallService,
 		removeRelease,
 		removeApp,
 	],

tests/orchestrator/planning.spec.ts

@@ -404,7 +404,10 @@ describe('orchestrator/planning', () => {
 			plan()
 				.action("initialize release 'r1' for app 'a0'")
 				.action("pull image 'alpine:latest' with tag 'a0_main:r1'")
-				.action("migrate unchanged service 'main' of app 'a0 to release 'r1' '")
+				.action("migrate unchanged service 'main' of app 'a0 to release 'r1'")
+				.action(
+					"remove metadata for service 'main' from release 'r0' of app 'a0'",
+				)
 				.action("remove release 'r0'")
 				.action("pull image 'alpine:latest' with tag 'a0_other:r1'")
 				.action(

tests/orchestrator/tasks.ts

@@ -312,14 +312,29 @@ export const startService = Task.of<Device>().from({
 });
 
 /**
- * Rename a service between releases if the image and service configuration has not changed
+ * Delete service data from a release
  *
- * Condition: the service exists (it has a container id), the container is not running, the container configuration
- *            matches the target configuration, and source and target images are the same
- * Effect: move the service from the source release to the target release
+ * This is used to migrate a service between releases. It is only to be used within a method
+ */
+const deleteService = Task.of<Device>().from({
+	op: 'delete',
+	lens: '/apps/:appUuid/releases/:releaseUuid/services/:serviceName',
+	effect: () => {
+		/* the operation already tells the framework to delete afterwards */
+	},
+	description: ({ serviceName, appUuid, releaseUuid }) =>
+		`remove metadata for service '${serviceName}' from release '${releaseUuid}' of app '${appUuid}'`,
+});
+
+/**
+ * Rename a service container between releases if the image and service configuration has not changed
+ *
+ * This is called only as part of the migrateService method so we skip the condition.
+ *
+ * Effect: copy the service from the source release to the target release
  * Action: rename the container using the docker API
  */
-export const migrateService = Task.of<Device>().from({
+const renameServiceContainer = Task.of<Device>().from({
 	op: 'create',
 	lens: '/apps/:appUuid/releases/:releaseUuid/services/:serviceName',
 	condition: (
@@ -342,9 +357,6 @@ export const migrateService = Task.of<Device>().from({
 		const currRelease = Object.keys(releases).find((u) => u !== releaseUuid)!;
 		const currService = releases[currRelease]?.services[serviceName];
 
-		// Remove the release from the current release
-		delete releases[currRelease].services[serviceName];
-
 		// Move the service to the new release
 		service._ = currService;
 	},
@@ -356,18 +368,59 @@ export const migrateService = Task.of<Device>().from({
 		const currRelease = Object.keys(releases).find((u) => u !== releaseUuid)!;
 
 		const currService = releases[currRelease]?.services[serviceName];
-		delete releases[currRelease].services[serviceName];
 
 		// Rename the container
 		await docker.getContainer(currService.containerId!).rename({
 			name: getContainerName({ releaseUuid, serviceName }),
 		});
 
-		// Move the container to the new release
+		// Copy the container to the new release
 		service._ = currService;
 	},
 	description: (ctx) =>
-		`migrate unchanged service '${ctx.serviceName}' of app '${ctx.appUuid} to release '${ctx.releaseUuid}' '`,
+		`migrate unchanged service '${ctx.serviceName}' of app '${ctx.appUuid} to release '${ctx.releaseUuid}'`,
+});
+
+/**
+ * Rename a service container between releases if the image and service configuration has not changed
+ *
+ * This is a method as changes on two different releases need to be performed.
+ * The method will first rename the container and link the service on the target release. It then will
+ * remove the service metadata from the current release
+ *
+ * Condition: the service exists (it has a container id), the container configuration
+ *            matches the target configuration, and source and target images are the same
+ */
+export const migrateService = Task.of<Device>().from({
+	op: 'create',
+	lens: '/apps/:appUuid/releases/:releaseUuid/services/:serviceName',
+	expansion: 'sequential',
+	condition: (
+		_,
+		{ appUuid, releaseUuid, serviceName, system: device, target },
+	) => {
+		const { releases } = device.apps[appUuid];
+		const [currentRelease] = Object.keys(releases).filter(
+			(u) => u !== releaseUuid,
+		);
+		const currService = releases[currentRelease]?.services[serviceName];
+		return (
+			currService != null &&
+			isEqualConfig(currService, target) &&
+			target.image === currService.image
+		);
+	},
+	method: (
+		_,
+		{ system: device, appUuid, serviceName, releaseUuid, target },
+	) => {
+		const { releases } = device.apps[appUuid];
+		const currRelease = Object.keys(releases).find((u) => u !== releaseUuid)!;
+		return [
+			renameServiceContainer({ target, appUuid, serviceName, releaseUuid }),
+			deleteService({ appUuid, releaseUuid: currRelease, serviceName }),
+		];
+	},
 });
 
 /**
@@ -436,7 +489,7 @@ export const stopService = Task.of<Device>().from({
  * Effect: remove the service from the device state
  * Action: remove the container using the docker API
  */
-export const removeService = Task.of<Device>().from({
+export const uninstallService = Task.of<Device>().from({
 	op: '*',
 	lens: '/apps/:appUuid/releases/:releaseUuid/services/:serviceName',
 	condition: (service) =>