-
Notifications
You must be signed in to change notification settings - Fork 315
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
Fix container running check to work for windows hosts #470
Conversation
manifests/run.pp
Outdated
@@ -320,7 +320,7 @@ | |||
} else { | |||
exec { "start ${title} with docker": | |||
command => "${docker_command} start ${sanitised_title}", | |||
onlyif => "${docker_command} inspect ${sanitised_title} -f \"{{ .State.Running }}\" | grep false", | |||
onlyif => "${docker_command} inspect ${sanitised_title} -f \"{{ if (.State.Running) }} {{ nil }}{{ end }}\"", |
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.
so it is the same command in both places. the check is different, ie onlyif vs unless. Am i correct in that summation ?
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.
was supposed to be a revert from #451 but I just realised this does not work also. that's why dave changed it to use grep but we don't have that on windows so i'll have to find a command that works on all OSs...
EDIT - Given that this is supposed to be a revert of #451 because it broke on Windows, highlights the need for testing on a Windows host to stop this kind of thing. |
@glennsarti Good catch about the Readme |
Given the functionality you're leveraging, you can make a really good argument that testing on 2016 would satisfy the 2019 requirement i.e. it's already mostly tested and not worth the effort to to acceptance testing on 2019. |
Great work @florindragos ! 👍 |
No description provided.