Fall back to name/source for matching software titles on insert if bundle ID is provided, add missing special case for edit software upload timeout #22412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ndle ID is provided This avoids "no rows in set" errors when adding software whose name/source matches an existing software title but whose bundle ID doesn't, likely because the bundle ID is null due to e.g. a GitOps upload. There's likely more work to do (e.g. on VPP) for this fallback, and on the write side we should probably be saving bundle identifiers in more places than we're doing (e.g. in the batch endpoint where GitOps is used) but this is the smallest fix that will un-break software uploads.
|
Test failures are unrelated to this change. |
Missed this in dev. This fixes package uploads on software edit timing out on lower-speed internet connections on larger packages.
iansltx
changed the title
3 tasks
iansltx
changed the base branch from
minor-fleet-v4.57.0
to
patch-fleet-v4.57.1
iansltx
commented
Sep 26, 2024
There was a problem hiding this comment.
Got some subtle edge cases here, and I want to say the data store tests would pass as they stand pre-patch, hence the need to add the cases I outlined.
Thinking that we actually want to constrain the query on the app side for bundle ID more, so it checks for source ID, so I'm going to push that change in a sec.
| app *fleet.VPPApp | ||
| }{ | ||
| { | ||
| name: "title that already exists, no bundle identifier in payload", |
There was a problem hiding this comment.
See notes on the installers test cases.
There was a problem hiding this comment.
Confirmed that this catches bugginess in 4.57.0, and we have a test for mismatched bundle ID in the other test (not this one)...but probably might as well add one more test case since this is a different code path than for installers. Going to work on adding that now.
iansltx
force-pushed
the
quickfix-21370
branch
from
2f4185e
to
aafb59c
Compare
|
I'm happy with the way the tests look now, so this comment is my signoff on @roperzh's work (can't approve since I created the PR and did some of the work). @lucasmrod can you take one more look? I believe you're ready here, and code between this and #22413 are identical. |
|
Test failures are known unrelated flakiness fixed in |
|
Closing this out so we can follow proper patch procedure (#24213 into main, cherry-picked into patch release from there). |
roperzh
added a commit
that referenced
this pull request
Sep 26, 2024
… bundle ID, add missing request timeout special case for edit package endpoint (#22413) Same as #22412, for #21370, but against `main` rather than 4.57.0. # Checklist for submitter If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Roberto Dip <rroperzh@gmail.com>
georgekarrv
pushed a commit
that referenced
this pull request
Sep 26, 2024
… bundle ID, add missing request timeout special case for edit package endpoint (#22413) Same as #22412, for #21370, but against `main` rather than 4.57.0. If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Roberto Dip <rroperzh@gmail.com>
georgekarrv
pushed a commit
that referenced
this pull request
Oct 1, 2024
… bundle ID, add missing request timeout special case for edit package endpoint (#22413) Same as #22412, for #21370, but against `main` rather than 4.57.0. If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality --------- Co-authored-by: Roberto Dip <rroperzh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #21370
For the first fix:
This avoids "no rows in set" errors when adding software whose name/source matches an existing software title but whose bundle ID doesn't, likely because the bundle ID is null due to e.g. a GitOps upload.
There's likely more work to do (e.g. on VPP) for this fallback, and on the write side we should probably be saving bundle identifiers in more places than we're doing (e.g. in the batch endpoint where GitOps is used) but this is the smallest fix that will un-break software uploads.
For the second fix: this matches the timeout for PATCH to the timeout for POST on installers, so if e.g. you're on a 20 Mbps up connection uploading a 150MB installer the upload actually completes rather than timing out after 20s.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
@roperzh lmk if this looks right. Pending assignment on this, I can add tests if this is the right approach.