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

Fix container running check to work for windows hosts #470

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

florindragos
Copy link
Contributor

No description provided.

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 }}\"",
Copy link
Contributor

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 ?

Copy link
Contributor Author

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...

@glennsarti
Copy link

glennsarti commented Apr 9, 2019

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.

@florindragos
Copy link
Contributor Author

@glennsarti Good catch about the Readme
We haven't tested on Windows 2019 and I don't have any knowledge of someone requesting it (yet) but we should certainly add support for it.

@glennsarti
Copy link

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.

@tphoney
Copy link
Contributor

tphoney commented Apr 16, 2019

Great work @florindragos ! 👍

@tphoney tphoney merged commit 1cbcf1e into puppetlabs:master Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants