{"action":"created","comment":{"url":"https://api.github.com/repos/puppetlabs/puppet/pulls/comments/92301427","pull_request_review_id":12814152,"id":92301427,"diff_hunk":"@@ -288,5 +288,25 @@ def set(param, value)\n       raise Puppet::Error, \"Could not set #{param} on #{@resource.class.name}[#{@resource.name}]: #{detail}\", detail.backtrace\n     end\n   end\n+\n+  # From overriding Puppet::Property#insync?\n+  # Ruby Etc::getpwnam < 2.1.0 always returns a struct with binary encoded\n+  # string values, and >= 2.1.0 will return binary encoded strings for values\n+  # incompatible with current locale charset. Convert \"should\" value to binary\n+  # if \"current\" value is, to avoid unnecessary property syncs and concatenation\n+  # of strings with incompatible encodings. (PUP-6777)\n+  #\n+  # * \"should\" parameter will always be a string with one possible (strictly\n+  # speaking) exception, which is the boolean value true. In current semantics,\n+  # this would convert to a string. For strict backwards-compatibility, convert\n+  # it to a string if this is the case.\n+  #\n+  # return basic string comparison after encoding (same as\n+  # Puppet::Property#property_matches)\n+  def comments_insync?(current, should)\n+    should = should.to_s if should == true\n+    should.force_encoding(Encoding::ASCII_8BIT) if current.encoding == Encoding::ASCII_8BIT","path":"lib/puppet/provider/nameservice.rb","position":21,"original_position":21,"commit_id":"9bf7f7aae6dbf567916f62f94b01ddc00099d217","original_commit_id":"4078d2dbd4a4b7a0076d702d936bf71f933e2b82","user":{"login":"MosesMendoza","id":819133,"avatar_url":"https://avatars.githubusercontent.com/u/819133?v=3","gravatar_id":"","url":"https://api.github.com/users/MosesMendoza","html_url":"https://github.com/MosesMendoza","followers_url":"https://api.github.com/users/MosesMendoza/followers","following_url":"https://api.github.com/users/MosesMendoza/following{/other_user}","gists_url":"https://api.github.com/users/MosesMendoza/gists{/gist_id}","starred_url":"https://api.github.com/users/MosesMendoza/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MosesMendoza/subscriptions","organizations_url":"https://api.github.com/users/MosesMendoza/orgs","repos_url":"https://api.github.com/users/MosesMendoza/repos","events_url":"https://api.github.com/users/MosesMendoza/events{/privacy}","received_events_url":"https://api.github.com/users/MosesMendoza/received_events","type":"User","site_admin":false},"body":"Complicating this, I came across another case when running this through a new acceptance test, which is not covered by this code as-is:\r\n\r\nWhen using `puppet apply` our `is` value, when not binary, will be in `Encoding.default_external`. In the case of our Ubuntu 16.04 acceptance hosts, that is `ISO-8859-1`. Perhaps this lends support to trying to set the encoding of `should` to `is` for comparison. In terms of error messaging, we could possibly rescue `EncodingError` with a more helpful error message.","created_at":"2016-12-14T00:49:42Z","updated_at":"2016-12-14T00:49:43Z","html_url":"https://github.com/puppetlabs/puppet/pull/5419#discussion_r92301427","pull_request_url":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419","_links":{"self":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/comments/92301427"},"html":{"href":"https://github.com/puppetlabs/puppet/pull/5419#discussion_r92301427"},"pull_request":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419"}}},"pull_request":{"url":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419","id":97406771,"html_url":"https://github.com/puppetlabs/puppet/pull/5419","diff_url":"https://github.com/puppetlabs/puppet/pull/5419.diff","patch_url":"https://github.com/puppetlabs/puppet/pull/5419.patch","issue_url":"https://api.github.com/repos/puppetlabs/puppet/issues/5419","number":5419,"state":"open","locked":false,"title":"(PUP-6777) handle binary encoding from Etc in nameservice provider","user":{"login":"MosesMendoza","id":819133,"avatar_url":"https://avatars.githubusercontent.com/u/819133?v=3","gravatar_id":"","url":"https://api.github.com/users/MosesMendoza","html_url":"https://github.com/MosesMendoza","followers_url":"https://api.github.com/users/MosesMendoza/followers","following_url":"https://api.github.com/users/MosesMendoza/following{/other_user}","gists_url":"https://api.github.com/users/MosesMendoza/gists{/gist_id}","starred_url":"https://api.github.com/users/MosesMendoza/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MosesMendoza/subscriptions","organizations_url":"https://api.github.com/users/MosesMendoza/orgs","repos_url":"https://api.github.com/users/MosesMendoza/repos","events_url":"https://api.github.com/users/MosesMendoza/events{/privacy}","received_events_url":"https://api.github.com/users/MosesMendoza/received_events","type":"User","site_admin":false},"body":"The nameservice provider leverages the ruby Etc::getpwnam method to return\r\ninformation regarding users (subclasses include useradd and pw). In Ruby <\r\n2.1.0, this method always returns values as strings in binary encoding. In Ruby\r\n2.1.0 and later, this method returns strings in either the current locale\r\nencoding (Encoding::default_external) OR binary encoding if the string returned\r\ndoes not encode correctly in the current locale encoding (how they determine\r\nthis is opaque, at best, and not exposed publicly).\r\n\r\nPrior to this commit, the :comment property of the user type would set the\r\nexternal encoding of the supplied value for :comment to binary if running ruby\r\n< 2.1.0. This effectively handles the case of incompatible encodings with ruby\r\n< 2.1.0. However, for ruby >= 2.1.0, this will always fail if the value of\r\n:comment contains character(s) that are not represented in the current locale,\r\nbecause ruby Etc will supply this 'is' value in binary and we will compare it\r\nto our 'should' value in whatever the current locale is, failing.\r\n\r\nThis commit instead overrides the Puppet::Property#insync? method for\r\nPuppet::Type::User::Comment when using the nameservice provider, specifically\r\nsingling out the case where the value returned by Etc::getpwnam is in binary\r\nencoding. In this case, we encode our \"should\" value in binary. This should\r\neffectively handle all scenarios as our \"should\" value will always be encoded\r\nin Encoding.default_external, and the system \"is\" value will always either be\r\nin binary or Encoding.default_external.\r\n\r\nUnfortunately, Puppet::Type::User::Comment does no value validation, which\r\nmeans there is some odd and undefined behavior regarding acceptable value\r\ntypes. In theory, it is possible to supply a string, boolean (true only) or\r\narray to as the value for a user comment. The value true is silently converted\r\nto the string 'true' and when 'setting' a value via an array the first element\r\nis used. When 'checking' a value with an array, any of the elements matching is\r\ndeemed 'in sync'. Managing the comment property with an array makes no sense to\r\nme, so this commit leverages Puppet::Property#should (which, given an array\r\neffectively returns the first element in this case) instead of passing the\r\narray @should and handling that in the new method. However, I can see a\r\nterrible world where someone meant their comment => true to mean comment =>\r\n'true' and I don't want to break that poor soul's user management until our\r\nnext major, so we convert true to 'true'.\r\n\r\nSigned-off-by: Moses Mendoza <moses@puppet.com>","created_at":"2016-12-10T01:20:11Z","updated_at":"2016-12-14T00:49:43Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b118c99ca1342f32502c30e8a92d3a642490625c","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419/commits","review_comments_url":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419/comments","review_comment_url":"https://api.github.com/repos/puppetlabs/puppet/pulls/comments{/number}","comments_url":"https://api.github.com/repos/puppetlabs/puppet/issues/5419/comments","statuses_url":"https://api.github.com/repos/puppetlabs/puppet/statuses/9bf7f7aae6dbf567916f62f94b01ddc00099d217","head":{"label":"MosesMendoza:PUP-6777/master/handle_etc_encoding_in_nameservice_provider","ref":"PUP-6777/master/handle_etc_encoding_in_nameservice_provider","sha":"9bf7f7aae6dbf567916f62f94b01ddc00099d217","user":{"login":"MosesMendoza","id":819133,"avatar_url":"https://avatars.githubusercontent.com/u/819133?v=3","gravatar_id":"","url":"https://api.github.com/users/MosesMendoza","html_url":"https://github.com/MosesMendoza","followers_url":"https://api.github.com/users/MosesMendoza/followers","following_url":"https://api.github.com/users/MosesMendoza/following{/other_user}","gists_url":"https://api.github.com/users/MosesMendoza/gists{/gist_id}","starred_url":"https://api.github.com/users/MosesMendoza/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MosesMendoza/subscriptions","organizations_url":"https://api.github.com/users/MosesMendoza/orgs","repos_url":"https://api.github.com/users/MosesMendoza/repos","events_url":"https://api.github.com/users/MosesMendoza/events{/privacy}","received_events_url":"https://api.github.com/users/MosesMendoza/received_events","type":"User","site_admin":false},"repo":{"id":3267256,"name":"puppet","full_name":"MosesMendoza/puppet","owner":{"login":"MosesMendoza","id":819133,"avatar_url":"https://avatars.githubusercontent.com/u/819133?v=3","gravatar_id":"","url":"https://api.github.com/users/MosesMendoza","html_url":"https://github.com/MosesMendoza","followers_url":"https://api.github.com/users/MosesMendoza/followers","following_url":"https://api.github.com/users/MosesMendoza/following{/other_user}","gists_url":"https://api.github.com/users/MosesMendoza/gists{/gist_id}","starred_url":"https://api.github.com/users/MosesMendoza/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/MosesMendoza/subscriptions","organizations_url":"https://api.github.com/users/MosesMendoza/orgs","repos_url":"https://api.github.com/users/MosesMendoza/repos","events_url":"https://api.github.com/users/MosesMendoza/events{/privacy}","received_events_url":"https://api.github.com/users/MosesMendoza/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/MosesMendoza/puppet","description":"Server automation framework and application","fork":true,"url":"https://api.github.com/repos/MosesMendoza/puppet","forks_url":"https://api.github.com/repos/MosesMendoza/puppet/forks","keys_url":"https://api.github.com/repos/MosesMendoza/puppet/keys{/key_id}","collaborators_url":"https://api.github.com/repos/MosesMendoza/puppet/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/MosesMendoza/puppet/teams","hooks_url":"https://api.github.com/repos/MosesMendoza/puppet/hooks","issue_events_url":"https://api.github.com/repos/MosesMendoza/puppet/issues/events{/number}","events_url":"https://api.github.com/repos/MosesMendoza/puppet/events","assignees_url":"https://api.github.com/repos/MosesMendoza/puppet/assignees{/user}","branches_url":"https://api.github.com/repos/MosesMendoza/puppet/branches{/branch}","tags_url":"https://api.github.com/repos/MosesMendoza/puppet/tags","blobs_url":"https://api.github.com/repos/MosesMendoza/puppet/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/MosesMendoza/puppet/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/MosesMendoza/puppet/git/refs{/sha}","trees_url":"https://api.github.com/repos/MosesMendoza/puppet/git/trees{/sha}","statuses_url":"https://api.github.com/repos/MosesMendoza/puppet/statuses/{sha}","languages_url":"https://api.github.com/repos/MosesMendoza/puppet/languages","stargazers_url":"https://api.github.com/repos/MosesMendoza/puppet/stargazers","contributors_url":"https://api.github.com/repos/MosesMendoza/puppet/contributors","subscribers_url":"https://api.github.com/repos/MosesMendoza/puppet/subscribers","subscription_url":"https://api.github.com/repos/MosesMendoza/puppet/subscription","commits_url":"https://api.github.com/repos/MosesMendoza/puppet/commits{/sha}","git_commits_url":"https://api.github.com/repos/MosesMendoza/puppet/git/commits{/sha}","comments_url":"https://api.github.com/repos/MosesMendoza/puppet/comments{/number}","issue_comment_url":"https://api.github.com/repos/MosesMendoza/puppet/issues/comments{/number}","contents_url":"https://api.github.com/repos/MosesMendoza/puppet/contents/{+path}","compare_url":"https://api.github.com/repos/MosesMendoza/puppet/compare/{base}...{head}","merges_url":"https://api.github.com/repos/MosesMendoza/puppet/merges","archive_url":"https://api.github.com/repos/MosesMendoza/puppet/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/MosesMendoza/puppet/downloads","issues_url":"https://api.github.com/repos/MosesMendoza/puppet/issues{/number}","pulls_url":"https://api.github.com/repos/MosesMendoza/puppet/pulls{/number}","milestones_url":"https://api.github.com/repos/MosesMendoza/puppet/milestones{/number}","notifications_url":"https://api.github.com/repos/MosesMendoza/puppet/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/MosesMendoza/puppet/labels{/name}","releases_url":"https://api.github.com/repos/MosesMendoza/puppet/releases{/id}","deployments_url":"https://api.github.com/repos/MosesMendoza/puppet/deployments","created_at":"2012-01-25T18:29:08Z","updated_at":"2016-11-30T22:00:16Z","pushed_at":"2016-12-13T01:30:29Z","git_url":"git://github.com/MosesMendoza/puppet.git","ssh_url":"git@github.com:MosesMendoza/puppet.git","clone_url":"https://github.com/MosesMendoza/puppet.git","svn_url":"https://github.com/MosesMendoza/puppet","homepage":"http://www.puppetlabs.com/puppet/introduction/","size":53265,"stargazers_count":1,"watchers_count":1,"language":"Ruby","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":1,"default_branch":"master"}},"base":{"label":"puppetlabs:master","ref":"master","sha":"609247058e942eadcd562beb1ea19f86adf65f40","user":{"login":"puppetlabs","id":234268,"avatar_url":"https://avatars.githubusercontent.com/u/234268?v=3","gravatar_id":"","url":"https://api.github.com/users/puppetlabs","html_url":"https://github.com/puppetlabs","followers_url":"https://api.github.com/users/puppetlabs/followers","following_url":"https://api.github.com/users/puppetlabs/following{/other_user}","gists_url":"https://api.github.com/users/puppetlabs/gists{/gist_id}","starred_url":"https://api.github.com/users/puppetlabs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/puppetlabs/subscriptions","organizations_url":"https://api.github.com/users/puppetlabs/orgs","repos_url":"https://api.github.com/users/puppetlabs/repos","events_url":"https://api.github.com/users/puppetlabs/events{/privacy}","received_events_url":"https://api.github.com/users/puppetlabs/received_events","type":"Organization","site_admin":false},"repo":{"id":910744,"name":"puppet","full_name":"puppetlabs/puppet","owner":{"login":"puppetlabs","id":234268,"avatar_url":"https://avatars.githubusercontent.com/u/234268?v=3","gravatar_id":"","url":"https://api.github.com/users/puppetlabs","html_url":"https://github.com/puppetlabs","followers_url":"https://api.github.com/users/puppetlabs/followers","following_url":"https://api.github.com/users/puppetlabs/following{/other_user}","gists_url":"https://api.github.com/users/puppetlabs/gists{/gist_id}","starred_url":"https://api.github.com/users/puppetlabs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/puppetlabs/subscriptions","organizations_url":"https://api.github.com/users/puppetlabs/orgs","repos_url":"https://api.github.com/users/puppetlabs/repos","events_url":"https://api.github.com/users/puppetlabs/events{/privacy}","received_events_url":"https://api.github.com/users/puppetlabs/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/puppetlabs/puppet","description":"Server automation framework and application","fork":false,"url":"https://api.github.com/repos/puppetlabs/puppet","forks_url":"https://api.github.com/repos/puppetlabs/puppet/forks","keys_url":"https://api.github.com/repos/puppetlabs/puppet/keys{/key_id}","collaborators_url":"https://api.github.com/repos/puppetlabs/puppet/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/puppetlabs/puppet/teams","hooks_url":"https://api.github.com/repos/puppetlabs/puppet/hooks","issue_events_url":"https://api.github.com/repos/puppetlabs/puppet/issues/events{/number}","events_url":"https://api.github.com/repos/puppetlabs/puppet/events","assignees_url":"https://api.github.com/repos/puppetlabs/puppet/assignees{/user}","branches_url":"https://api.github.com/repos/puppetlabs/puppet/branches{/branch}","tags_url":"https://api.github.com/repos/puppetlabs/puppet/tags","blobs_url":"https://api.github.com/repos/puppetlabs/puppet/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/puppetlabs/puppet/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/puppetlabs/puppet/git/refs{/sha}","trees_url":"https://api.github.com/repos/puppetlabs/puppet/git/trees{/sha}","statuses_url":"https://api.github.com/repos/puppetlabs/puppet/statuses/{sha}","languages_url":"https://api.github.com/repos/puppetlabs/puppet/languages","stargazers_url":"https://api.github.com/repos/puppetlabs/puppet/stargazers","contributors_url":"https://api.github.com/repos/puppetlabs/puppet/contributors","subscribers_url":"https://api.github.com/repos/puppetlabs/puppet/subscribers","subscription_url":"https://api.github.com/repos/puppetlabs/puppet/subscription","commits_url":"https://api.github.com/repos/puppetlabs/puppet/commits{/sha}","git_commits_url":"https://api.github.com/repos/puppetlabs/puppet/git/commits{/sha}","comments_url":"https://api.github.com/repos/puppetlabs/puppet/comments{/number}","issue_comment_url":"https://api.github.com/repos/puppetlabs/puppet/issues/comments{/number}","contents_url":"https://api.github.com/repos/puppetlabs/puppet/contents/{+path}","compare_url":"https://api.github.com/repos/puppetlabs/puppet/compare/{base}...{head}","merges_url":"https://api.github.com/repos/puppetlabs/puppet/merges","archive_url":"https://api.github.com/repos/puppetlabs/puppet/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/puppetlabs/puppet/downloads","issues_url":"https://api.github.com/repos/puppetlabs/puppet/issues{/number}","pulls_url":"https://api.github.com/repos/puppetlabs/puppet/pulls{/number}","milestones_url":"https://api.github.com/repos/puppetlabs/puppet/milestones{/number}","notifications_url":"https://api.github.com/repos/puppetlabs/puppet/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/puppetlabs/puppet/labels{/name}","releases_url":"https://api.github.com/repos/puppetlabs/puppet/releases{/id}","deployments_url":"https://api.github.com/repos/puppetlabs/puppet/deployments","created_at":"2010-09-14T19:26:44Z","updated_at":"2016-12-12T10:23:55Z","pushed_at":"2016-12-13T21:54:10Z","git_url":"git://github.com/puppetlabs/puppet.git","ssh_url":"git@github.com:puppetlabs/puppet.git","clone_url":"https://github.com/puppetlabs/puppet.git","svn_url":"https://github.com/puppetlabs/puppet","homepage":"https://puppet.com/puppet/what-is-puppet","size":53662,"stargazers_count":4269,"watchers_count":4269,"language":"Ruby","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":1756,"mirror_url":null,"open_issues_count":61,"forks":1756,"open_issues":61,"watchers":4269,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419"},"html":{"href":"https://github.com/puppetlabs/puppet/pull/5419"},"issue":{"href":"https://api.github.com/repos/puppetlabs/puppet/issues/5419"},"comments":{"href":"https://api.github.com/repos/puppetlabs/puppet/issues/5419/comments"},"review_comments":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419/comments"},"review_comment":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/puppetlabs/puppet/pulls/5419/commits"},"statuses":{"href":"https://api.github.com/repos/puppetlabs/puppet/statuses/9bf7f7aae6dbf567916f62f94b01ddc00099d217"}}}}
{"actor":{"display_login":"MosesMendoza"}}