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.

Sign up for GitHub

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

Jump to bottom

Fix Unicode-objects must be encoded before hashing #46608

Merged
merged 1 commit into from
Dec 11, 2018
Merged

Fix Unicode-objects must be encoded before hashing #46608

merged 1 commit into from
Dec 11, 2018

Conversation

daixijun
Copy link
Contributor

@daixijun daixijun commented Oct 8, 2018

SUMMARY

with azure_rm plugin: Unicode-objects must be encoded before hashing

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

inventory plugin
azure_rm

ANSIBLE VERSION
ansible 2.7.0
  config file = /Users/xijun/projects/ansible/xxx/azure/ansible.cfg
  configured module search path = ['/Users/xijun/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.7.0 (default, Aug 22 2018, 15:22:33) [Clang 9.1.0 (clang-902.0.39.2)]
ADDITIONAL INFORMATION
$ cat xxx.azure_rm.yaml
plugin: azure_rm
cloud_environment: AzureChinaCloud
include_vm_resource_groups:
  - verystar
auth_source: auto # credential_file

$ ansible all -i xxx.azure_rm.yaml --list-hosts -vvvv
 [WARNING]:  * Failed to parse /Users/xijun/projects/ansible/xxx/azure/xxx.azure_rm.yaml with azure_rm plugin: Unicode-objects must be encoded before hashing

 [WARNING]: Unable to parse /Users/xijun/projects/ansible/xxx/azure/xxx.azure_rm.yaml as an inventory source

 [WARNING]: No inventory was parsed, only implicit localhost is available

 [WARNING]: provided hosts list is empty, only localhost is available. Note that the implicit localhost does not match 'all'

  hosts (0):

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. python3 small_patch support:community This issue/PR relates to code supported by the Ansible community. labels Oct 8, 2018
@ryansb ryansb removed needs_triage Needs a first human triage before being processed. labels Oct 9, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 16, 2018
@pmarques
Copy link
Contributor

this LGTM

shipit

@ansibot ansibot added the community_review In order to be merged, this PR must follow the community review workflow. label Oct 26, 2018
@yungezz
Copy link
Contributor

yungezz commented Oct 29, 2018

@nitzmahone I met same error when trying azure_rm plugin, could you pls help to review the PR? thanks.

@pmarques
Copy link
Contributor

bot_status

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 24, 2018
@yungezz
Copy link
Contributor

yungezz commented Nov 26, 2018

@nitzmahone Could you pls help to review the change? thanks.

webknjaz
webknjaz approved these changes Nov 29, 2018
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

LGTM overall

lib/ansible/plugins/inventory/azure_rm.py
@@ -450,7 +450,7 @@ def __init__(self, vm_model, inventory_client, vmss=None):
self.nics = []

# Azure often doesn't provide a globally-unique filename, so use resource name + a chunk of ID hash
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(vm_model['id']).hexdigest()[0:4])
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4])
Copy link
Member

Choose a reason for hiding this comment

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

How about multylining it for readability?

Suggested change
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4])
self.default_inventory_hostname = (
'_'.join((
vm_model['name'],
hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4],
))
)

lib/ansible/plugins/inventory/azure_rm.py
@@ -450,7 +450,7 @@ def __init__(self, vm_model, inventory_client, vmss=None):
self.nics = []

# Azure often doesn't provide a globally-unique filename, so use resource name + a chunk of ID hash
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(vm_model['id']).hexdigest()[0:4])
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4])
Copy link
Member

Choose a reason for hiding this comment

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

Or maybe

Suggested change
self.default_inventory_hostname = '{0}_{1}'.format(vm_model['name'], hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4])
self.default_inventory_hostname = '_'.join((
vm_model['name'],
hashlib.sha1(to_bytes(vm_model['id'])).hexdigest()[0:4],
))

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 7, 2018
@blark
Copy link

blark commented Dec 10, 2018

Plugin is broken now, I have the same error. Would be nice to have a fix ASAP. Thanks!

@pmarques
Copy link
Contributor

@daixijun can you close and open the PR to re-run the CI?

@nitzmahone nitzmahone merged commit 7186234 into ansible:devel Dec 11, 2018
@blark
Copy link

blark commented Dec 11, 2018

Thanks. Working now! How long do things like this usually take to make it back into stable?

kbreit pushed a commit to kbreit/ansible that referenced this pull request Jan 11, 2019
@daixijun @kbreit
Fix Unicode-objects must be encoded before hashing (ansible#46608)
0a427b3
@f3rdy
Copy link

f3rdy commented Mar 6, 2019 •
edited

@nitzmahone I'd really like to see this in stable-2.7,and maybe a new 2.7-release until 2.8 is fully availble on pypi. Is this, by any chance possible or planned?

@webknjaz
Copy link
Member

webknjaz commented Mar 6, 2019

@f3rdy I think it should be ok if you send a backport PR

@lukasmrtvy
Copy link

@f3rdy Will You send a PR for 2.7?

f3rdy added a commit to f3rdy/ansible that referenced this pull request Apr 5, 2019
@f3rdy
fixing ansible#46608 for private stable-2.7
26acb43
@f3rdy f3rdy mentioned this pull request Apr 5, 2019
Closed
@f3rdy
Copy link

f3rdy commented Apr 5, 2019

@f3rdy Will You send a PR for 2.7?

Done.

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pmarques pmarques pmarques approved these changes

@webknjaz webknjaz webknjaz approved these changes

@yungezz yungezz yungezz approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. new_contributor This PR is the first contribution by a new community member. python3 small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@daixijun @pmarques @yungezz @blark @f3rdy @webknjaz @lukasmrtvy @ryansb @ansibot @nitzmahone