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

Edible Scripts Backend #25739

Merged
merged 9 commits into from
Jan 30, 2025
Merged

Edible Scripts Backend #25739

merged 9 commits into from
Jan 30, 2025

Conversation

dantecatalfamo
Copy link
Member

@dantecatalfamo dantecatalfamo commented Jan 23, 2025

#24602

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
  • Added/updated automated tests

@dantecatalfamo dantecatalfamo changed the title Edible Scripts Edible Scripts Backend Jan 23, 2025
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 57.14286% with 48 lines in your changes missing coverage. Please review.

Project coverage is 63.58%. Comparing base (ff43593) to head (306f1f1).
Report is 84 commits behind head on main.

Files with missing lines Patch % Lines
server/service/scripts.go 53.33% 29 Missing and 13 partials ⚠️
server/datastore/mysql/scripts.go 71.42% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25739      +/-   ##
==========================================
- Coverage   63.59%   63.58%   -0.01%     
==========================================
  Files        1622     1625       +3     
  Lines      155023   155573     +550     
  Branches     3967     3967              
==========================================
+ Hits        98580    98923     +343     
- Misses      48662    48834     +172     
- Partials     7781     7816      +35     
Flag Coverage Δ
backend 64.44% <57.14%> (-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.

@dantecatalfamo dantecatalfamo marked this pull request as ready for review January 27, 2025 14:57
@dantecatalfamo dantecatalfamo requested a review from a team as a code owner January 27, 2025 14:57
@georgekarrv
Copy link
Member

Sounds delicious 🍰

Comment on lines 769 to 772
val := r.MultipartForm.Value["id"]
if len(val) < 1 {
return nil, &fleet.BadRequestError{Message: "no script id"}
}
Copy link
Contributor

@sgress454 sgress454 Jan 28, 2025

Choose a reason for hiding this comment

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

Do we typically require the entity ID to be part of the body in a PATCH request? It's already present in the URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, I'll take another look at this

Copy link
Contributor

@sgress454 sgress454 left a comment

Choose a reason for hiding this comment

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

Tested 👍 , code and tests look good, just the one question about requiring script ID in the body.

@dantecatalfamo
Copy link
Member Author

@sgress454 Looks like it wasn't even needed, I wasn't sure if URL decoding was handled when we do a custom DecodeRequest

@dantecatalfamo
Copy link
Member Author

Tests failing, maybe I'll have to add it back

server/service/scripts.go Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@sgress454 sgress454 left a comment

Choose a reason for hiding this comment

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

Tested 👍

@dantecatalfamo dantecatalfamo merged commit 3c8033f into main Jan 30, 2025
35 checks passed
@dantecatalfamo dantecatalfamo deleted the 24602-editable-scripts branch January 30, 2025 18:01
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.

3 participants