-
Notifications
You must be signed in to change notification settings - Fork 13
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
Raise an error when updating a runtime currently in use. #2885
Conversation
mitchell-as
commented
Nov 20, 2023
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-2091 We ensure the runtime is not in use before updating it |
Test failures are not due to this PR. |
pkg/platform/runtime/setup/setup.go
Outdated
@@ -220,6 +235,35 @@ func (s *Setup) Update() (rerr error) { | |||
return nil | |||
} | |||
|
|||
func (s *Setup) getProcessesInUse() map[string]int32 { |
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 think it makes more sense for this function to be part of the osutils
package and we can supply it the directory to compare. Also we could have os-specific versions of these functions to reduce the if statements here.
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.
Okay, I moved it into the osutils
package. However, when I tried splitting it up into OS-specific functions, I ended up with too much duplication. I left it as a single function.
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.
Looks good, is there an easy way for us to introduce an integration test for this? If it's too much it can be a follow-up story. Just a nice to have right now.