Service installs: Fix creating two tasks when moving a device #1857
Conversation
thgreasi
force-pushed
the
fix-double-task-on-move
branch
4 times, most recently
from
574e94d
to
342f406
Compare
thgreasi
force-pushed
the
fix-double-task-on-move
branch
2 times, most recently
from
8793dbc
to
8bf34dc
Compare
| // We could also have an optimization for the case that `values.is_pinned_on__release != null` | ||
| // to make the part that finds the target release faster, but chose to keep this simpler since: | ||
| // a) We expect that in the majority of device move requests users will not be also be pinning | ||
| // the device at the same time. | ||
| // b) Only the sync approach would benefit from it, since creating the service installs via tasks | ||
| // (which is going to be the default), only accepts the deviceIds as a parameter, |
There was a problem hiding this comment.
This is very much a TODO comment and would make more sense in the commit body imo, having it in the code like this is probably not going to end up being useful as no one will look at it anyway. Fwiw I do agree we don't need the extra levels of optimization as well so the only reason we would ever act on this is if we found performance issues that triggered us to investigate anyway, in which case we probably wouldn't find this
| await createAppServiceInstalls(api, newAppId, affectedIds, tx); | ||
| return; |
There was a problem hiding this comment.
I'd appreciate a comment saying that since we're moving apps we can just create the app service installs and not worry about targeting the specific release, otherwise I would wonder why we're returning here
Change-type: patch Signed-off-by: Thodoris Greasidis <[email protected]>
Change-type: patch
Change-type: patch
thgreasi
force-pushed
the
fix-double-task-on-move
branch
from
8bf34dc
to
be81dab
Compare
|
🤦 I just rebased before finding out that it was already approved w/ comments... |
Before this, we had two device PATCH hooks, (a) one handling device moves, and (b) one handling device pinning&unpinning, but we also have (c) a separate hook that unpins devices when moved. As a result when moving a device (c) would change the values so that the payload includes both
belongs_to__application&is_pinned_on__release, hence both (a) & (b) hook would run and try to create any missing service install, which other than the extra work, it also means that if task based service install creation was enabled, would cause two tasks to be added.We could also have an optimization for the case that
values.is_pinned_on__release != nullto make the part that finds the target release faster, but chose to keep this simpler since:
a) We expect that in the majority of device move requests users will not be also be pinning
the device at the same time.
b) Only the sync approach would benefit from it, since creating the service installs via tasks
(which is going to be the default), only accepts the deviceIds as a parameter.
Change-type: patch