Tests for self service vpp apps & weird installed vpp app edge case by ksykulev · Pull Request #28372 · fleetdm/fleet

ksykulev · 2025-04-18T15:50:40Z

When a vpp app is marked as self service, it should be returned and the data about it should be hydrated.
When writing tests for this, another bug was found around vpp apps that were installed and present in host software. These apps need to be retrieved and the data about them needs to be merged onto the host hostInstalledSoftware record.

Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
Added/updated automated tests
A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
Manual QA for all new/changed functionality
For unreleased bug fixes in a release candidate, confirmed that the fix is not expected to adversely impact load test results or alerted the release DRI if additional load testing is needed.

When a vpp app is marked as self service, it should be returned and the data about it should be hydrated. When writing tests for this, another bug was found around vpp apps that were installed and present in host software. These apps need to be retrieved and the data about them needs to be merged onto the host `hostInstalledSoftware` record.

codecov · 2025-04-18T16:04:39Z

Codecov Report

Attention: Patch coverage is 92.10526% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.99%. Comparing base (261b244) to head (9caab30).
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/software.go	92.10%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #28372      +/-   ##
==========================================
+ Coverage   63.98%   63.99%   +0.01%     
==========================================
  Files        1785     1785              
  Lines      171339   171546     +207     
  Branches     4945     4945              
==========================================
+ Hits       109629   109788     +159     
- Misses      53061    53099      +38     
- Partials     8649     8659      +10

Flag	Coverage Δ
backend	`65.04% <92.10%> (+0.01%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

iansltx · 2025-04-21T08:48:47Z

 }

+func hostInstalledVpps(ds *Datastore, ctx context.Context, hostID uint) ([]*hostSoftware, error) {
+	vppInstalledStmt := `


We're mising label scoping here :(

If the VPP app got installed and later got dropped out of label scope, it should show up as in inventory and neither as installed nor as available for install.

This will continue being an issue when we revise host install scoping, though at that point self-service will be filtered by labels rather than overall availability for install.

Should I cover the scoping as part of #28328 in a new PR?

Or would you prefer if I do it in this PR for vpp installs and then cover the rest of the cases missed in the new PR?

Can handle scoping in a new PR. I'll give this one a thumbs-up momentarily for merge with the knowledge that we have a separate bug tracked for that work.

iansltx · 2025-04-21T08:50:57Z

+        INSERT INTO software (name, version, source, bundle_identifier, title_id, checksum)
+        VALUES (?, ?, ?, ?, ?, ?)
+	`,
+		vPPApp2.Name, vPPApp2.LatestVersion, "apps", vPPApp2.BundleIdentifier, vPPApp2.TitleID, hex.EncodeToString([]byte("vpp2")),


Should pick a different value than latestVersion here for version, as we want to make sure in assertions that inventory shows the inventoried version rather than the VPP app latest version.

iansltx · 2025-04-21T20:35:42Z

+					if softwareTitleRecord.SoftwareIDList == nil {
+						softwareTitleRecord.SoftwareIDList = ptr.String("")
+						softwareTitleRecord.SoftwareSourceList = ptr.String("")
+						softwareTitleRecord.VersionList = ptr.String("")
+						softwareTitleRecord.BundleIdentifierList = ptr.String("")
+						seperator = ""
+					}
+
+					if !strings.Contains(*softwareTitleRecord.SoftwareIDList, softwareIDStr) {
+						*softwareTitleRecord.SoftwareIDList += seperator + softwareIDStr
+						*softwareTitleRecord.SoftwareSourceList += seperator + s.Source
+						*softwareTitleRecord.VersionList += seperator + *s.Version
+						*softwareTitleRecord.BundleIdentifierList += seperator + *s.BundleIdentifier
+					}


This makes me a bit nervous that we're intentionally passing around concatenated lists of strings rather than a proper slice structure, but guessing this was the least diruptive way to add the lists we need, and the lists added above are used not only in L3746-3749 but appended to/defined elsewhere, so we have to stick with this format to avoid a bigger refactor?

Fixed this! Great callout. Thank you.

iansltx

Approving with the assumption that the code I commented on is as good as we can get for now., and given that labels work is coming in its own PR. Will re-review if anything changes here on either of those, but expecting this'll get merged instead.

iansltx

Thanks for fixing the contains issue.

…28372) #28345 When a vpp app is marked as self service, it should be returned and the data about it should be hydrated. When writing tests for this, another bug was found around vpp apps that were installed and present in host software. These apps need to be retrieved and the data about them needs to be merged onto the host `hostInstalledSoftware` record. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it) - [x] Manual QA for all new/changed functionality - [x] For unreleased bug fixes in a release candidate, confirmed that the fix is not expected to adversely impact load test results or alerted the release DRI if additional load testing is needed.

…28431) Cherry pick #28372 Original issue #28345

#28328 Also related to #28372 - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Added/updated automated tests - [x] A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it) - [x] Manual QA for all new/changed functionality - [x] For unreleased bug fixes in a release candidate, confirmed that the fix is not expected to adversely impact load test results or alerted the release DRI if additional load testing is needed.

cherry pick #28432 issue #28328 Also related to #28372

ksykulev requested a review from a team as a code owner April 18, 2025 15:50

ksykulev temporarily deployed to Docker Hub April 18, 2025 15:50 — with GitHub Actions Inactive

iansltx reviewed Apr 21, 2025

View reviewed changes

ensure InstalledVersions is always added, even with prior vpp installs

326fc0a

ksykulev temporarily deployed to Docker Hub April 21, 2025 19:23 — with GitHub Actions Inactive

iansltx reviewed Apr 21, 2025

View reviewed changes

iansltx previously approved these changes Apr 21, 2025

View reviewed changes

slice instead of substring

9caab30

ksykulev dismissed iansltx’s stale review via 9caab30 April 21, 2025 21:04

ksykulev temporarily deployed to Docker Hub April 21, 2025 21:04 — with GitHub Actions Inactive

iansltx approved these changes Apr 21, 2025

View reviewed changes

ksykulev merged commit eeb1cbd into main Apr 22, 2025
38 checks passed

ksykulev deleted the 28345-tests branch April 22, 2025 03:53

This was referenced Apr 22, 2025

Tests for self service vpp apps & weird installed vpp app edge case #28431

Merged

Applying label scope to vpp apps #28432

Merged

ksykulev added a commit that referenced this pull request Apr 22, 2025

Tests for self service vpp apps & weird installed vpp app edge case (#…

4dad3f9

…28431) Cherry pick #28372 Original issue #28345

ksykulev mentioned this pull request Apr 22, 2025

Applying label scope to vpp apps (#28432) #28467

Merged

ksykulev added a commit that referenced this pull request Apr 22, 2025

Applying label scope to vpp apps (#28432) (#28467)

702bdb7

cherry pick #28432 issue #28328 Also related to #28372

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests for self service vpp apps & weird installed vpp app edge case#28372

Tests for self service vpp apps & weird installed vpp app edge case#28372
ksykulev merged 3 commits intomainfrom
28345-tests

ksykulev commented Apr 18, 2025 •

edited

Loading

Uh oh!

codecov Bot commented Apr 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Uh oh!

ksykulev Apr 21, 2025

Uh oh!

iansltx Apr 21, 2025

Uh oh!

ksykulev Apr 22, 2025

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Uh oh!

ksykulev Apr 21, 2025

Uh oh!

iansltx left a comment

Uh oh!

iansltx left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ksykulev commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

ksykulev Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

iansltx Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

ksykulev Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iansltx Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

ksykulev Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

iansltx left a comment

Choose a reason for hiding this comment

Uh oh!

iansltx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ksykulev commented Apr 18, 2025 •

edited

Loading

codecov Bot commented Apr 18, 2025 •

edited

Loading