-
Notifications
You must be signed in to change notification settings - Fork 60
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
Update device on group membership change #3330
Update device on group membership change #3330
Conversation
@ZJvandeWeg not specifically adding you as a reviewer, but I thought to tag you as docs around device-groups will be updated as per comment here: #3318 (comment) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3330 +/- ##
==========================================
+ Coverage 40.92% 41.25% +0.32%
==========================================
Files 607 608 +1
Lines 22762 22865 +103
Branches 5494 5535 +41
==========================================
+ Hits 9315 9432 +117
+ Misses 13447 13433 -14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@Steve-Mcl think I'd like to do a huddle review of this PR with you on Monday. This may be building on choices already made, but I'd like to understand better why the device group needs the |
Sure, NP. For the memory banks and later discussion:
|
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.
We have since merged a migration with a newer filename to main. This migration will need renaming.
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.
done 9e88a2c
@knolleary review ready following changes discussed. OP updated, tests updated etc. |
// check to see if the group is now empty | ||
const remainingDevices = await deviceGroup.deviceCount() | ||
if (remainingDevices === 0) { | ||
deviceGroup.targetSnapshotId = null |
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.
deviceGroup.targetSnapshotId = null | |
deviceGroup.targetSnapshotId = null |
Wondering if setting the snapshot to null when the group is emptied is the right behaviour. A user could be in the middle of changing the devices in the group (albeit doing it in multiple steps) - and clearing the snapshot mid-way through the task could be unexpected.
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.
I don't think it is of consequence.
it just means the pipeline needs to be executed to make it re-sync.
The issue this was added for is adding devices to a spare empty group (months/years later) and suddently they fire up running old (dangerous?) snapshots.
I feel quite strongly that we DONT actually provide a checkbox [x] remove current snapshot
/ [x] apply current snapshot
- I think it is dangerous and suspect most (other) users will not like the behaviour we are adding at all
From the ISSUE #3260 (comment):
@MarianRaphael I believe it would be prudent to consider this:
Personally, I believe this should be done via an option when adding a device to a group since you may NOT actually want this to occur immediately and without warning that "things" could start moving/happening.
Consider the scenario:
You have x devices that you need to add to a device group BUT FIRST you need to update the source snapshot
The current snapshot is NOT what needs to be deployed (because changes have not yet been made to accommodate the 10 new devices)
You CANNOT/DO NOT want to deploy the new changes to the existing devices without the other devices being online/ready to receive the new snapshot.
By simply adding a checkbox "[x] Auto Deploy current device group snapshot 'version 2.1'?" at the time of adding the device to a group can make this problem go away!
@@ -432,6 +432,9 @@ module.exports = { | |||
// Update the targetSnapshot of the device group | |||
await targetDeviceGroup.PipelineStageDeviceGroup.update({ targetSnapshotId: sourceSnapshot.id }, { transaction }) | |||
|
|||
// Update the targetSnapshotId on the device group | |||
await targetDeviceGroup.update({ targetSnapshotId: sourceSnapshot.id }, { transaction }) | |||
|
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.
A possibly cleaner approach would be to have a controller for DeviceGroup which provides a function to update the targetSnapshotId which takes care of updating the devices and sending the update command. As it is we've got that logic spread through this Pipeline controller. Not blocking, just a thought.
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.
I could add a method to DeviceGroup controller but I dont want to be updating devices until the transaction is committed.
closes #3260
Description
Premise: The customer wishes for devices added to a group to receive the snapshot last pushed via a pipeline. Additionally, the customer wishes for the pipeline deployed snapshot to be removed from the device when it is removed from the group.
Adds fieldactivePipelineStageId
toDeviceGroups
so that added/removed devices can query the active pipeline stage and be updated accordinglytargetSnapshotId
toDeviceGroups
so that added/removed devices can query the DeviceGroup and be updated accordinglyAdds: All Devices removed get an update request and clears activePipelineStageIdDeviceGroup.targetSnapshotId
Related Issue(s)
#3260
Checklist
flowforge.yml
?FlowFuse/helm
to update ConfigMap TemplateFlowFuse/CloudProject
to update values for Staging/ProductionLabels
backport
labelarea:migration
label