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

Timeout in instance stop #150

Merged
merged 6 commits into from
Mar 24, 2020
Merged

Timeout in instance stop #150

merged 6 commits into from
Mar 24, 2020

Conversation

pierlauro
Copy link
Contributor

Added ability to specify stop timeout (default 10 - as for singularity instance stop specification)

@vsoch
Copy link
Member

vsoch commented Mar 24, 2020

okay, looks like this goes back to at least 3.1 (and there are no docs for 3.0) so the check for version seems sufficient to honor the variable. Should we perhaps instead of doing a default, only add the variable given the case that the user has defined it and we have Singularity version 3? This would be akin to honoring the Singularity default (and how it might change over time) instead of hard coding it here. Other than that small change, if you can confirm this works for you, I'm okay with not adding a test for now because it seems like it would be challenging to easily do (unless you have an idea?)

spython/instance/cmd/stop.py Outdated Show resolved Hide resolved
@pierlauro
Copy link
Contributor Author

okay, looks like this goes back to at least 3.1 (and there are no docs for 3.0) so the check for version seems sufficient to honor the variable. Should we perhaps instead of doing a default, only add the variable given the case that the user has defined it and we have Singularity version 3? This would be akin to honoring the Singularity default (and how it might change over time) instead of hard coding it here. Other than that small change, if you can confirm this works for you, I'm okay with not adding a test for now because it seems like it would be challenging to easily do (unless you have an idea?)

I totally agree on the proposed change, this way (1) older singularity versions would not be affected and (2) there will be no problem if singularity changes the default timeout.

spython/instance/cmd/stop.py Outdated Show resolved Hide resolved
@pierlauro
Copy link
Contributor Author

Regarding the testing, as we're just making a call to singularity stop (that is already working) with an additional argument, I think it is not worth instrumenting some complex to setup time-based test

@vsoch
Copy link
Member

vsoch commented Mar 24, 2020

Agree! And your changes look good.

@vsoch vsoch merged commit 6c9e7a6 into singularityhub:master Mar 24, 2020
@vsoch
Copy link
Member

vsoch commented Mar 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants