-
Notifications
You must be signed in to change notification settings - Fork 60
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
FR-5826 - Make sure we cleanup properly after update-grub #164
Conversation
Codecov Report
@@ Coverage Diff @@
## main #164 +/- ##
==========================================
- Coverage 90.62% 90.52% -0.11%
==========================================
Files 13 13
Lines 2636 2628 -8
==========================================
- Hits 2389 2379 -10
- Misses 215 216 +1
- Partials 32 33 +1
|
7218a63
to
153b89f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. I actually think that this way of testing this is very creative, so it's okay. I don't think stdout would be corrupted here somehow, so it's fine. Inline comment is more of a question comment, so feel free to merge it if you don't think it needs action.
internal/statemachine/helper.go
Outdated
// detach the loopback device | ||
teardownCmds = append(teardownCmds, | ||
//nolint:gosec,G204 | ||
execCommand("losetup", "--detach", loopUsed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, +1, just made me think of something. Please note that I am not an experienced Go developer, so I'm fine if you say it's not the way to go. I was thinking: since the associateLoopDevice
command basically attaches the loop device, one thing that comes to my mind would for the actual function to return the command that needs to be executed to 'clean up' when we're done. Not sure if that's a scheme that's frequent in Golang though. What do you think? This or having the detach bits as a separate helper function, so that people know that once you associate, you have to unassociate as well at some point.
Also, not very important, just overthinking things!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you, I will do this. Moreover it will follow the same pattern used for the mount/unmount. It also makes sense to apply this to the dpkg-divert command I think.
I am not a golang expert either, but I think it looks fine to return a function to be called to cleanup things (using closure is rather idiomatic I think).
No description provided.