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 lastpass lookup error #48804

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@arnested

arnested commented Nov 16, 2018

SUMMARY

The lastpass lookup plugin fails with

An unhandled exception occurred while running the lookup plugin 'lastpass'. Error was a <class 'TypeError'>, original message: memoryview: a bytes-like object is required, not 'str'"

Fixes #42062.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lastpass

ADDITIONAL INFORMATION

Due to my limited Python experience this fix is primarily based on the way similar stuff is done in the onepass plugin (and testing of course).

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 16, 2018

Hi @arnested, thank you for submitting this pull-request!

click here for bot help

@arnested

This comment has been minimized.

arnested commented Nov 16, 2018

This failed due to different behaviour on python 2 and 3.

From the Shippable tests it appears to have succeeded on python 2, but not on python 3.

I have it running locally with python 3.7.1 (Shippable is 3.7.0 it appears).

This is probably beyond my understanding of Python (tried to Google around to get wiser -- no success yet 😄).

@abadger

This comment has been minimized.

Member

abadger commented Nov 16, 2018

I took a look and I believe this will fix things. However, there are some other problems with the separation of bytes and text in this plugin which will affect whether this is the right fix or not.

subprocess.Popen deals in byte strings. This plugin is wrapping that in a method, _run(). _run() is used by both get_fields() and logged_in. logged_in is self contained. It doesn't take parameters from the user or return output from _run() to the caller . So changing it to use byte strings is a valid way to fix things. It is more efficient as there is no conversion then.

get_fields is taking input from the user and returning output to the caller. So that method should be taking text strings and returning text strings. (Currently, it returns byte strings)

When we fix get_fields, the question becomes where should we turn bytes into text? We could turn the return values into text in get_fields in the return on line 82 or we could change the return value into text in _run() before returning out and err. If we fix it in _run() then everything will use text which is generally the right default for our code. We would not need to change the comparison in logged_in() in this case (or we would need to change it to u""). If we fix it in get_fields(), then it will only change the output from get_fields() so we would need to change get_fields() plus this PR.

Which one do we want to do?

@arnested arnested force-pushed the arnested:lastpass-lookup-failure branch from 34ca612 to 51a86a1 Nov 19, 2018

@acozine acozine added the ci_verified label Nov 27, 2018

@ansibot ansibot added the stale_ci label Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment