-
Notifications
You must be signed in to change notification settings - Fork 313
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 permission denied issue introduced in v4.2.0 #820
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Overriding $HOME unconditionally is probably a bad idea. This is still a draft so probably needed for some tests, but flag it so that we don't forget about it by mistake.
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.
This seems to be true for other command definitions. It's a temporary override but I do see your point.
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.
Please please please do not create new directories in /.
Please pick some other place (for example under /usr/local/share, /opt, /var/cache, or anywhere else but /).
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.
Hey @bjvrielink, I think you may have misinterpreted the proposed change here.
This PR removes the directory creation.
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.
Yeah, now I noticed. I've spent a little too much time right now trying to figure out what was/is broken in the current release.
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.
I did some runtime tests with the new changes, and line 27 of lib/puppet/provider/docker_compose/ruby.rb was failing as it wasn't getting the new tmpdir param.
I got it to work by changing line 27 of lib/puppet/provider/docker_compose/ruby.rb to
however, it is not idempotent as the TMPDIR var gets set every time the resource runs. I can't find a way to silence it.
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.
Good spot!
Interesting regarding the idempotency. I wouldn't have expected it to have an impact.
It's also highly possible that my suggested implementation is not the right fit here.
Maybe we shouldn't be overridden TMPDIR at all given that it it isn't actually a docker setting.
Could this boil down to a simple documentation update where we advise that impacted users may want to override TMPDIR with another puppet resource?
@smortex @kenyon what are your thoughts?
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.
I tried setting the TMPDIR env var previously using another puppet resource. However without a reboot the docker_compose resource wasn't seeing/respecting it. I like your draft approach as it takes care of it close to the compose resource, it just needs to occur silently if the param is declared - interestingly the existing (breaking) approach did that, however my testing with a hybrid method wasn't reading the resource[:tmpdir] if set earlier.
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.
Indeed so the resource hash is not available inside the has_command block so we can't use the environment method inside it.
That's why I ended up wrapping the command exec. Maybe Env.store isn't the right way to go about it 🧐.
The environment method did not seem to be available outside of that scope either (at least from inside a pry session).. could be wrong about that though.
I'm not by a computer right now so can't do any testing until later on.
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.
An alternative approach may be to precede the docker-compose command with 'TMPDIR=resource[:tmpdir]' so that it occurs as part of the command if the tmpdir param is declared. That way ruby isn't doing it, the shell is. Just a thought.
Setting the env var within the resource (and it subsequently being ephemeral) also avoids setting it globally on the system.