20404: Edit packages feature #21812
20404: Edit packages feature #21812
Conversation
RachelElysia
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21812 +/- ##
==========================================
- Coverage 65.34% 65.18% -0.17%
==========================================
Files 1493 1494 +1
Lines 116651 117013 +362
Branches 3452 3458 +6
==========================================
+ Hits 76229 76277 +48
- Misses 33316 33622 +306
- Partials 7106 7114 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
frontend/pages/SoftwarePage/components/AddPackageForm/helpers.ts
Outdated
Show resolved
Hide resolved
|
@RachelElysia Other note here is I'm assuming that you won't pass the |
RachelElysia
had a problem deploying
to
Docker Hub
GitHub Actions
Failure
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
36db6f8
to
ef60c57
Compare
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
ef60c57
to
5138dba
Compare
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
5138dba
to
fa739a2
Compare
RachelElysia
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
frontend/pages/SoftwarePage/components/AddPackageForm/helpers.ts
Outdated
Show resolved
Hide resolved
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
d558b61
to
3346907
Compare
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
c35fd8d
to
e0302c1
Compare
|
@lukeheath / @iansltx thanks for finishing up this feature! I'm unavailable to work on this any more but these are the TODOs I left. Hopefully it's not too much, and hopefully Jacob's work being merged in already will make addressing all this much quicker Frontend TODOs left (address and remove
Other TODOs:
Thank you both so much and hope this all goes relatively smoothly. |
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
e0302c1
to
2b4e45e
Compare
| @@ -237,7 +237,7 @@ func (ds *Datastore) SaveInstallerUpdates(ctx context.Context, payload *fleet.Up | |||
| } | |||
|
|
|||
| var postInstallScriptID *uint | |||
| if payload.PostInstallScript != nil && *payload.PostInstallScript != "" { // pointer because optional | |||
| if payload.PostInstallScript != nil { // pointer because optional | |||
There was a problem hiding this comment.
If the script is empty, we should set its ID to NULL in software_installers
There was a problem hiding this comment.
Yep, just realized that that was the previous behavior (postInstallScriptID *uint zero value being nil in L239). I've reverted the commit made based on your previous feedback. I had actually tested bouncing this back and forth and thought I had missed something when you brought the feedback up initially.
There was a problem hiding this comment.
Just verified this manually, going from blank post-install to populated, populated to different populated, and populated to blank. Values changed in the DB, including to/from NULL on post-install script contents ID.
This reverts commit fdd6f2d. Post install script ID is by default nil, which is what we want for a blank script, so the post-install script check was correct.
| /> | ||
| ) | ||
| } | ||
| fileDetails={tokenFile ? { name: tokenFile.name } : undefined} |
There was a problem hiding this comment.
Looks like the graphic name may have been accidentally removed here?
There was a problem hiding this comment.
It's passed in from line 134
| /> | ||
| ) | ||
| } | ||
| fileDetails={tokenFile ? { name: tokenFile.name } : undefined} |
There was a problem hiding this comment.
Looks like the graphic name may have been accidentally removed here?
There was a problem hiding this comment.
Passed in from line 102
| /> | ||
| ) | ||
| } | ||
| fileDetails={certFile ? { name: certFile.name } : undefined} |
There was a problem hiding this comment.
Looks like the graphic name may have been accidentally removed here?
There was a problem hiding this comment.
Confirmed the code passes line 115 graphicName in <FileDetails/>
| /> | ||
| ) | ||
| } | ||
| fileDetails={tokenFile ? { name: tokenFile.name } : undefined} |
There was a problem hiding this comment.
Looks like the graphic name may have been accidentally removed here?
There was a problem hiding this comment.
It's passed in from line 69
| /> | ||
| ) | ||
| } | ||
| fileDetails={tokenFile ? { name: tokenFile.name } : undefined} |
There was a problem hiding this comment.
Looks like the graphic name may have been accidentally removed here?
There was a problem hiding this comment.
Passed in from line 79
|
|
||
| val, ok = r.MultipartForm.Value["uninstall_script"] | ||
| if ok && len(val) > 0 { | ||
| decoded.UninstallScript = &val[0] |
There was a problem hiding this comment.
Isn't the same val variable being reused multiple times in this function? I think the pointer is going to get overwritten (e.g., the install script and post-install script in the decoded map would become the same as the uninstall script)
There was a problem hiding this comment.
Hmm, thanks for bringing this up (and for the playground link you DM'd me). What's weird is this actually seems to work when multiple scripts are changed, so I need to figure out what's going on. I think it works because the pointers are to the index in the multipart array, rather than to the val itself, so it's brittle but functional. Going to hack on the playground example to see if I can repro.
There was a problem hiding this comment.
Yep, this works because the pointer is to the 0th array element rather than val itself, so reassigning val doesn't overwrite the pointer. See this playground.
There was a problem hiding this comment.
Went ahead and renamed values that involve pointers so this is a bit less brittle.
iansltx
dismissed stale reviews from gillespi314 and rachaelshaw
via
f14e9ee
iansltx
force-pushed
the
20404-edit-software-fe
branch
from
22e5d18
to
f14e9ee
Compare
|
@rachaelshaw @gillespi314 I messed up here. Hit "update from main" post-cut and just backed that out because that would include items from post-cut in the cherry pick, which we don't want. Will need an approval on this again due to that dismissal. Sorry for the inconvenience. |
Because of how pointers work, the previous implementation wasn't shadowing values, but this is clearer/safer
|
Okay, that worked. Self-approval standing in for @gillespi314 because she already approved the same code, and it was my mistake that cleared the approval (this is not me approving me own code :) ). |
## Issue Cerra #20404 ## Description - Add frontend/API backend for editing software packages. GitOps will be a separate PR. ## More - Please see subtasks for change lists - #21611 - #21613 # 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] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] Manual QA for all new/changed functionality Automated tests will follow in another PR. --------- Co-authored-by: Ian Littman <[email protected]> Co-authored-by: Luke Heath <[email protected]> Co-authored-by: Jacob Shandling <[email protected]> Co-authored-by: Victor Lyuboslavsky <[email protected]>
(This cherry-pick is only of 21812, unreleased bugs found in QA process will be cherry picked separately for trackability on zenhub) > [!NOTE] > This PR is already merged in `main`, see #21812 This is against the release branch so it can be included in 4.57.0 ``` 1106 git fetch fleetdm 1107 git checkout fleetdm/minor-fleet-v4.57.0 1108 git log main 1109 git cherry-pick 1c1ebef 1110 git status 1111 git checkout -b 20404-edit-software-rc 1112 git push -u fleetdm 20404-edit-software-rc ``` --------- Co-authored-by: Ian Littman <[email protected]> Co-authored-by: Luke Heath <[email protected]> Co-authored-by: Jacob Shandling <[email protected]> Co-authored-by: Victor Lyuboslavsky <[email protected]> Co-authored-by: Rachael Shaw <[email protected]>
Issue
Cerra #20404
Description
More
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.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Automated tests will follow in another PR.