Skip to content
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 Device Group env vars API support #4659

Merged
merged 21 commits into from
Oct 21, 2024
Merged

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Oct 18, 2024

closes #4658

Description

Route added

  • PUT /api/v1/applications/:applicationId/device-groups/:groupId/settings
    • Expects body to contain an env array of objects

NOTE: Adding endpoint vs updating the PUT /api/v1/applications/:applicationId/device-groups/:groupId route
was a toss of a coin. Instances overload the PUT /api/v1/projects/:id route allowing for the
settings to be updated in the same request. However, devices have a separate route PUT /api/v1/devices/:deviceId route.
Since devices and device groups are closer in relation, I decided to follow suit. I did however structure this is a way that we can easily remove this endpoint and map the feature to the existing route if so desired

Specified behaviour

  • Only an owner role user can update the group settings
    • The RBAC application:device-group:update permission was re-used for this purpose
  • The env array of objects is merged with the existing member devices only in the live API (when a device is requesting settings)
  • Updating a device group model settings and saving will trigger all member devices to update their settings hash
  • When the hash is computed, group env vars are merged with the device env vars before computing the hash (they are not persisted in the device model)

Tests added

  Audit Log > Application
    ✔ Provides a logger for updating device group settings
  Application Device Groups API
    Update Device Settings
      ✔ Owner can update a device group settings (414ms)
      ✔ Merges new Group Env Var (351ms)
      ✔ Only updates a device settings hash if the group env var change affects the merged settings env (327ms)
      ✔ Member can not update a device group settings (403) (95ms)
      ✔ Non Member can not update a device group settings (404) (95ms)

Related Issue(s)

Owner: #4658
Parent: #4538

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 93.45794% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.59%. Comparing base (eb4d998) to head (50590fb).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
forge/ee/db/controllers/DeviceGroup.js 83.33% 3 Missing ⚠️
forge/ee/routes/applicationDeviceGroups/index.js 91.30% 2 Missing ⚠️
...rations/20241016-01-add-settings-to-devicegroup.js 80.00% 1 Missing ⚠️
forge/db/models/DeviceGroup.js 97.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4659      +/-   ##
==========================================
+ Coverage   78.53%   78.59%   +0.06%     
==========================================
  Files         303      304       +1     
  Lines       14399    14470      +71     
  Branches     3285     3303      +18     
==========================================
+ Hits        11308    11373      +65     
- Misses       3091     3097       +6     
Flag Coverage Δ
backend 78.59% <93.45%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - one pending question about a commented out function. Approved pending resolution of the question.

forge/ee/db/controllers/DeviceGroup.js Outdated Show resolved Hide resolved
@knolleary knolleary mentioned this pull request Oct 21, 2024
10 tasks
@Steve-Mcl
Copy link
Contributor Author

Thanks Nick.
I've merged the UI into this branch to make this a one time push to main.

I will do final a ux test on pre-staging instance & use that to grab screenshots for changelog.

Once I get docs done & approved, I'll merge this.

@Steve-Mcl Steve-Mcl merged commit 210bb21 into main Oct 21, 2024
14 checks passed
@Steve-Mcl Steve-Mcl deleted the 4658-device-group-env-api branch October 21, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Database column and API for updating device group settings env
3 participants