-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add takeover action for some device types #35
base: master
Are you sure you want to change the base?
Conversation
Change-type: minor
0482414
to
f522a6b
Compare
The logic looks sound here, but it really jumps outside the logic framework in We also could expand the value of takeoverTargetVersion to be an array if necessary. |
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 balena-hup-action-utils/lib/index.ts Lines 122 to 128 in f522a6b
targetTakeoverVersion which is pretty close to what we do with the isTakeoverRequired
Please correct me if I'm misunderstanding your point |
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 What I meant by jumping out of the flow is two things:
So the definition for jetson-xavier-nx-device-emmc could be like below, which also could be an array of strings for multiple jumps.
I would say the key that makes this approach reasonable/possible is that getHUPActionType() returns only the name of the action. |
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.
I agree this is confusing I'll make the changes |
@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:
Then all we need to do is create a PR that sets the right version for those three DTs. |
This commit adds `minTakeoverVersion` to indicate a version jump that requires a takeover instead of `balenahup`. This is used by `getHupActionType` to identify the need for a takeover update.
@@ -72,7 +72,7 @@ export class HUPActionHelper { | |||
deviceType: string, | |||
currentVersion: string, | |||
targetVersion: string, | |||
) { | |||
): ActionName | 'takeover' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }; |
I'm not entirely sure what is the best course of action
Inspected the rework but have not tested yet. Overall looks good except for the comment above on ActionName. |
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