Add takeover action for some device types

[pipex]

This is a proof of concept on how we could repurpose this module to indicate that a takeover is needed between certain versions for some device types.

Change-type: minor

[kb2ma]

The logic looks sound here, but it really jumps outside the logic framework in getHupActionType(). One alternative might be to define takeoverTargetVersion as a property of ActionConfig, which helps indicate it is a special case. Then below where those properties are looked up in getHupActionType(), first test if it's a takeover -- If targetTakeoverVersion is defined, currentVersion is less than tTV, and targetVersion is greater than tTV, then change the actionName to 'takeover'. Otherwise the action remains 'balenahup'.

We also could expand the value of takeoverTargetVersion to be an array if necessary.

[pipex]

I agree that is another way to do it, but I think that also jumps outside the logic no? You would still need to check in the else clause

balena-hup-action-utils/lib/index.ts

Lines 122 to 128 in f522a6b

	} else {
	// Takeover overrides the checks below for the device type
	if (this.isTakeoverRequired(deviceType, currentVersion, targetVersion)) {
	return 'takeover';
	}
	actionName = 'balenahup';
	}

for the targetTakeoverVersion which is pretty close to what we do with the isTakeoverRequired

Please correct me if I'm misunderstanding your point

[kb2ma]

If takeoverTargetVersion is a property of ActionConfig, we could consider takeover as a special case for balenahup -- and that is essentially its purpose. So in getHUPActionType(), the actionName initially would be 'balenahup'. Then takeoverTargetVersion would be read where the other actionConfig version values currently are read. Finally, at the end of getHUPActionType() there are some validation conditionals. Maybe just before those add the logic to see if takeoverTargetVersion is being crossed for balenahup, and if so return 'takeover' instead.

What I meant by jumping out of the flow is two things:

• The current logic in getHUPActionType() returns immediately if isTakeoverRequired() is true
• The definition of DeviceTypeConfig includes a special case for takeover minTargetVersion, which I found confusing

So the definition for jetson-xavier-nx-device-emmc could be like below, which also could be an array of strings for multiple jumps.

		'jetson-xavier-nx-devkit-emmc': {
			balenahup: {
				// NOTE: this version is here as a placeholder for
				// testing. Replace with the correct version before merging
				takeoverTargetVersion: '5.1.45+rev1',

I would say the key that makes this approach reasonable/possible is that getHUPActionType() returns only the name of the action.

[pipex]

Yes, I like this, it makes takeover kind of a special case of balenahup instead of its own process which is a good way to model it.

takeover minTargetVersion, which I found confusing

I agree this is confusing

I'll make the changes

[kb2ma]

@pipex, I was thinking about how to stage this work so it's easy to pull the trigger when all the other pieces are ready. How about this plan:

1. Add all 3 device types: jetson-xavier, jetson-xavier-nx-devkit, jetson-xavier-nx-devkit-emmc
2. Set the targetTakeoverVersion to 99.99.99 so it won't be invoked
3. Merge this PR

Then all we need to do is create a PR that sets the right version for those three DTs.

[kb2ma]

Why not just make 'takeover' another ActionName? As is, this likely forces clients to add this extra option as well. For example, see balena-proxy services.action-backend.actions.config.ts.

[kb2ma]

After more review I see how this decision is a judgement call. I'd still rather keep the names simple for clients and modify the type definition for the ActionsConfig as needed (just for action?). A comment for ActionName will help explain how it's different.

[pipex]

That was the first thing I did, but I would have to create an additional item here

balena-hup-action-utils/lib/config.ts

Line 20 in 8f8a770

actions: {

For those actions minSourceVersion and minTargetVersion are required and those values don't really make sense for takeover (or I don't know what to use)

I guess I could omit takeover in the type definition here

balena-hup-action-utils/lib/types.ts

Line 34 in 8f8a770

actions: { [K in ActionName]: ActionConfig };

but that also seems just as dirty?

I'm not entirely sure what is the best course of action

[kb2ma]

Inspected the rework but have not tested yet. Overall looks good except for the comment above on ActionName.