-
Notifications
You must be signed in to change notification settings - Fork 316
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
Add tmpdir option to docker_compose #823
Conversation
docker_compose is a typeBreaking changes to this file WILL impact these 1 modules (exact match):Breaking changes to this file MAY impact these 1 modules (near match):This module is declared in 6 of 579 indexed public
|
|
Hey! It's taken ages to get round to this sorry! I think that directory management should be left to the user. We already have the tools available for this at makes sense to keep the domains isolated. I'd be interested in getting some other opinions on this change too. Personally I think it's a reasonable to have this as an optional part of the configuration but there may be other suggestions out there. Thanks for putting in the effort 😀 |
@chelnak yep, the pull request as it stands does not attempt directory creation and only warns if the (optional) declared tmpdir does not exist. It achieves the desired outcome without the issue from the previous merge. I'll delete my subsequent comment with the example of creating the dir to avoid confusion. See the proposed changes to |
# Check if the the tmpdir target exists | ||
Puppet.warning("#{resource[:tmpdir]} (defined as docker_compose tmpdir) does not exist") unless Dir.exist?(resource[:tmpdir]) | ||
# Set TMPDIR environment variable only if defined among resources and exists | ||
ENV['TMPDIR'] = resource[:tmpdir] if Dir.exist?(resource[:tmpdir]) |
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.
How/when is this variable reset to it's original value?
I suspect that this would have an impact with other types / provider depending on resource ordering:
foo { 'before': # ENV['TMPDIR'] #=> nil
}
docker_compose { 'bar':
path => '/here/or/there',
}
foo { 'after': # ENV['TMPDIR'] #=> '/here/or/there'
}
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.
@smortex the variable only appears to have effect within the resource, and also the resource collector and ordering in init.pp
ensures that docker_compose runs last, so my tests to put ENV['TMPDIR']
during run show it as unset or reflect the system variable within prior resources. It also does not affect system-wide env whether set or unset.
Class['docker::repos']
-> Class['docker::install']
-> Class['docker::config']
-> Class['docker::service']
-> Docker::Registry <||>
-> Docker::Image <||>
-> Docker::Run <||>
-> Docker_compose <||>
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.
@canihavethisone Thanks for that explanation. Makes sense to me.
@smortex What do you think?
Merging - changes are now optional and should not impact existing users. |
This pull request adds an optional tmpdir parameter to the docker_compose resource. It is useful on systems where the default /tmp directory is mounted with noexec, or consumers wish to specify an alternate tmpdir other than the system-wide environment variable. Note that it does not create the target directory, the consumer is responsible to ensure that the directory exists and is executable.
As well as adding the type from @chelnak example, I have added the following to the provider, which includes verification that the tmpdir target exists, warns and does not set the env var if not. If there is a better way to perform this in Ruby, please comment.
The changes pass unit tests and have been verified with the following manifest
Note1: this implementation is optional, and does not affect global TMPDIR environment variable (as tested on Linux) as it only applies within the docker_compose resource. I have also updated the readme to reflect that this is optional.
Note2: if any resource has the tmpdir param set, subsequent resources will inherit it unless they specify a different tmpdir target. Therefore it only has to be specified in the first resource if you want it to apply to all (as per above example, test2 will also use the same tmpdir, however if test2 was declared first it would use the system TMPDIR variable or default to /tmp if not set and fail if that dir is noexec). If subsequent resource tmpdir is defined but does not exist, a warning is raised and the previously successful tmpdir value is used.
Apologies for the previous attempt at this that also managed directory creation and introduced a breaking change on first puppet run (though other changes in that merge did fix idempotency with scaling). This however is a more considered approach to address issue as reported at docker/compose#8041 and experienced on hardened systems with noexec on /tmp.
I have not expanded the acceptance test for these changes, and defer to maintainers if they wish to add one.
Any comment or suggestions are welcome.