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

Added timeout and docker file directory #22

Merged
merged 5 commits into from Nov 7, 2013

Conversation

blarghmatey
Copy link

I added the option of passing a timeout parameter to the shell out method to prevent an erroneous failure due to long-running image builds. I also added the option to pass the directory that contains the dockerfile so that the ADD parameter in the dockerfile can be supported.

@@ -36,3 +36,4 @@
attribute :privileged, :kind_of => [TrueClass, FalseClass]
attribute :user, :kind_of => [String]
attribute :volume, :kind_of => [String]
attribute :cmd_timeout, :kind_of => [Integer]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not just set the :default => 60 here rather than all the || 60 for each shell_out? Also is there a specific reason for 60 seconds versus any other arbitrary value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as image: Should this just be called timeout instead? While the current implementation is shell_out, I think the feature should be abstracted away.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding in the :default => 60. The reason for choosing 60 is because it is the default value for the shell_out command, so I figured it would be reasonable to maintain that. As far as the timeout vs cmd_timeout, I had tried that at first but it was causing issues because it was overridden by a built in chef attribute.

@bflad
Copy link
Contributor

bflad commented Oct 21, 2013

A few changes in this pull request and I'll get this merged. Thanks for the contribution!

…dockerfile_directory attribute to be path instead.
@bflad
Copy link
Contributor

bflad commented Oct 21, 2013

Great thanks for the clarifications.

@bflad bflad merged commit 21e7a2d into sous-chefs:master Nov 7, 2013
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.

None yet

2 participants