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

feat: remove file if it exists before creating new one #15186

Merged
merged 7 commits into from
Nov 22, 2023

Conversation

jahzielv
Copy link
Contributor

@jahzielv jahzielv commented Nov 17, 2023

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
    See Changes files for more information.
  • Added/updated tests
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (5f313c8) 67.34% compared to head (a09b779) 67.35%.
Report is 3 commits behind head on main.

Files Patch % Lines
orbit/pkg/packaging/linux_shared.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15186   +/-   ##
=======================================
  Coverage   67.34%   67.35%           
=======================================
  Files        1021     1021           
  Lines       88118    88121    +3     
  Branches     2297     2297           
=======================================
+ Hits        59347    59350    +3     
  Misses      24351    24351           
  Partials     4420     4420           
Flag Coverage Δ
backend 68.68% <0.00%> (+<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.

@jahzielv
Copy link
Contributor Author

jahzielv commented Nov 17, 2023

@lucasmrod @mostlikelee @getvictor I was thinking about automated testing approaches for this fix, but haven't settled on anything satisfactory. The current packaging tests are not extensive fwiw, so maybe we don't need a new test for this?
One thought I had was to use testcontainers to verify that the package can be installed, but I haven't been able to make an ubuntu container work locally with this method. If y'all have any ideas I'd appreciate them!

@lucasmrod
Copy link
Member

#13260

Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

LGTM!

@lucasmrod
Copy link
Member

@lucasmrod @mostlikelee @getvictor I was thinking about automated testing approaches for this fix, but haven't settled on anything satisfactory. The current packaging tests are not extensive fwiw, so maybe we don't need a new test for this? One thought I had was to use testcontainers to verify that the package can be installed, but I haven't been able to make an ubuntu container work locally with this method. If y'all have any ideas I'd appreciate them!

How about testing buildNFPM? (Run that two times, first time with larger enroll than second, then unpack the secondly generated .deb and check that the enroll secret file matches the expected.)

Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

  • We might also need to test RPM packages (probably has the same issue?)
  • Adding the unit test.

@jahzielv jahzielv marked this pull request as ready for review November 22, 2023 14:32
@jahzielv jahzielv requested a review from a team as a code owner November 22, 2023 14:32
@jahzielv jahzielv requested a review from lucasmrod November 22, 2023 18:20
@jahzielv jahzielv merged commit 5131879 into main Nov 22, 2023
27 checks passed
@jahzielv jahzielv deleted the 13260-deb-installer branch November 22, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants