Service installs: Avoid creating duplicate tasks on device registration #1867
Conversation
…eation Change-type: patch
…registration Change-type: patch
| const addSystemAppServiceInstallHooks = ( | ||
| fieldName: 'should_be_managed_by__release' | 'should_be_operated_by__release', | ||
| ) => { | ||
| hooks.addPureHook('POST', 'resin', 'device', { |
There was a problem hiding this comment.
Moved this higher, in the hooks.addPureHook('POST', 'resin', 'device') that we have for user apps.
| // creates all service installs (target release, hostApp & SV release), so we don't need | ||
| // to run the extra code bellow to handle them separately, and doing so would create | ||
| // duplicate & unecessary tasks. | ||
| return; |
There was a problem hiding this comment.
Since I expect that soon tasks will become the only supported way for creating service installs, I decided to fix this in the simplest way that required the less changes in the current code.
| if (ASYNC_TASKS_ENABLED && ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED) { | ||
| // When creating service installs via the async tasks, a single call to createServiceInstallsAsync() | ||
| // creates all service installs (target release, hostApp & SV release), so we don't need | ||
| // to run the extra code bellow to handle them separately, and doing so would create |
There was a problem hiding this comment.
| // to run the extra code bellow to handle them separately, and doing so would create | |
| // to run the extra code below to handle them separately, and doing so would create |
| // When creating service installs via the async tasks, a single call to createServiceInstallsAsync() | ||
| // creates all service installs (target release, hostApp & SV release), so we don't need | ||
| // to run the extra code bellow to handle them separately, and doing so would create | ||
| // duplicate & unecessary tasks. |
There was a problem hiding this comment.
| // duplicate & unecessary tasks. | |
| // duplicate & unnecessary tasks. |
| @@ -260,6 +260,13 @@ hooks.addPureHook('POST', 'resin', 'device', { | |||
| const app = request.values.belongs_to__application; | |||
| if (app != null) { | |||
| await createAppServiceInstalls(rootApi, app, [deviceId], tx); | |||
| if (ASYNC_TASKS_ENABLED && ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED) { | |||
There was a problem hiding this comment.
Shouldn't ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED only ever equal true if ASYNC_TASKS_ENABLED is also true? Maybe there was a discussion about this at some point, but I feel like system init should fail if ASYNC_TASKS_ENABLED === false && ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED === true.
There was a problem hiding this comment.
Yea I agree on that this was strange. I expected it to only have to check one of the two, maybe something like:
export let ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED = ASYNC_TASKS_ENABLED && boolVar(
'ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED',
false,
);
But since I don't have background on why it was done this way, I just copied it "as is" from above where we do the same check. I guess that it might be this way to make mocking in tests easier. I would prefer to PR such a change separately and fix all occurrences and whichever tests might break after having a short chat on it.
Change-type: patch
thgreasi
force-pushed
the
fix-double-tasks
branch
from
10b2126
to
ad0c14d
Compare
Change-type: patch