(#9862) Do not manage the group if it doesn't exist#1344
Merged
hlindberg merged 14 commits intopuppetlabs:masterfrom Jan 2, 2013
Merged
Conversation
The previous tests around the owner behavior for a file setting did very little to test the logic around what actual owner will be used when. This commit changes the tests to make them more clearly define the behavior of the owner selection.
The previous tests for group behaviour of the file setting was not very clear as to the logic it tried to implement. This changes the tests and makes clear that the group behavior is a little odd in that if 'root' is specified it still uses the service group.
Now that the behavior around selecting the right owner and group is explicitly tested there is no need for the tests around the internal method to determine whether to use a service user or not. This removes those tests and fixes up the names of some of the replacement tests.
The previous error message for an invalid owner looked like: Error: Could not intialize global default settings: Internal error: The :owner setting for Where Puppet stores dynamic and growing data. The default for this setting is calculated specially, like `confdir`_.: vardir must be either 'root' or 'service', not 'gnats' That message was not very clear as to what had just gone wrong because it is providing far too much information. This commit changes the message too: Error: Could not intialize global default settings: The :owner parameter for the setting 'vardir' must be either 'root' or 'service', not 'gnats' This applies to both the owner and group parameters.
The error message for the group parameter had gotten out of sync with what the allowed values actually were. This updates the message to include all of the allowed values.
The previous behavior allowed the group parameter to be set to root, but it still selected the configured "service" group. This is contrary to what is done for the owner parameter and seems to have been an accidental behavior. This commit brings this portion of the group handling in line with user.
The previous behavior for managing the group of a file meant that if the group didn't exist puppet errored when trying to set permissions. This changes the behavior to check for the existance of the group and in the case of the group not existing (and we don't expect it to be created because `:mkusers` is false) then the group is not managed. There were several alternatives that were possible: * use the primary group of the "root" user * use the primary group of the `:owner` for the file Neither of these were compellingly better alternatives, since this should only be happening when puppet hasn't been fully configured on the system.
The logic for testing if a group or user were available tried to short-circuit, but ended up being less clear than if it just branched. It also created a variable that wasn't actually used later.
This checks to make sure that when puppet is run without a specific user/group present it doesn't fail. Instead it just ignores the given user. When a user/group is present then it will manage the ownership.
The previous tests were not very careful about making sure that they were going to be going down the right branches to test the path that they wanted. This makes some of the branch selection more explicit so make the tests a bit more robust to change.
Some core logic of the setting, which is how to manage the user and group, was hidden inside obtuse if conditions. This tries to make it clearer what is happening, at the expense of being longer, by promoting the different values available for owner and group to classes that can contain the logic for how to deal with that particular value.
lib/puppet/type/group.rb
Outdated
Contributor
There was a problem hiding this comment.
Suggest a proper yardoc comment when writing something new. Say, something like this:
# Returns whether the group exists or not by delegating to the associated provider. # @return [Boolean] whether the group exists or not.
What is the value of the comment "Yay breaking out of APIs" ?
Contributor
Author
There was a problem hiding this comment.
Sorry, just copy and pasted from what is done in user.rb. I should have changed that.
This adds YARD docs for methods and classes that are part of managing owners and groups of files (and directories).
hlindberg
added a commit
that referenced
this pull request
Jan 2, 2013
…ithout-puppet-group (#9862) Fix puppet cannot run without puppet group on the system
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The previous behavior for managing the group of a file meant that if the
group didn't exist puppet errored when trying to set permissions. This
changes the behavior to check for the existance of the group and in the
case of the group not existing (and we don't expect it to be created
because
:mkusersis false) then the group is not managed.There were several alternatives that were possible:
:ownerfor the fileNeither of these were compellingly better alternatives, since this should
only be happening when puppet hasn't been fully configured on the system.