onepassword_facts bug fixes

[Rylon]

SUMMARY

This PR fixes a couple of issues:

1. The session token wasn't being used properly for all lookups, so this prevented retrieving documents from 1Password items, even though retrieving fields worked.

2. The latest version of 1Password CLI changed the format of a response, so documents were not being retrieved properly.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

onepassword_facts

ADDITIONAL INFORMATION

The onepassword_facts module supports auto login via credentials passed in from a playbook, once the login is done, a session token is saved and re-used with each subsequent call to the 1Password CLI, however this was only being added in some cases, not in all cases.

[gundalow]

@Rylon Thank you for this PR

2. The latest version of 1Password CLI changed the format of a response, so documents were not being retrieved properly.

Does that mean that this PR would cause older versions (0.5.3?) to no longer work?

[Rylon]

Hi @gundalow actually, it’s worse than that, all older versions of 1Password CLI were “deprecated”, and no longer function. So at the moment the CLI doesn’t work at all, until upgrading it to v0.5.5, but even then the Ansible module will remain broken without this patch.

[gundalow]

@Rylon ah, thanks for the background, so in that case I'll merge this

[gundalow]

@Rylon Would you be able to raise a backport PR including a changelog/fragment for stable-2.7 by following https://docs.ansible.com/ansible/devel/community/development_process.html#making-your-pr-merge-worthy then we can get this fixed in the next 2.7 release (~ 3 weeks)

Thanks again!

[Rylon]

@gundalow Sure thing, I’ll try to take a look this weekend 👍

[Rylon]

Hi @gundalow sorry for the delay, I finally started to take a look at this and I noticed there's a odd issue in the currently shipping version in stable-2.7 where we've accidentally got two definitions for _run.

We have the same issue here as in #50160 where the module in 2.7 has diverged enough from devel that it's not possible to just cherry-pick the commits from devel, and I need to make a change to the _run method to support the new 1Password CLI.

Since currently the module doesn't work at all (any version older than v0.5.5 of 1Password's CLI was disabled), I'm proposing we just backport the entire module as it is in devel into stable-2.7, since I don't think there are any Ansible-specific changes made to it. I've tested it with this approach and it's working.

What do you all think @gundalow @samdoran @abadger ? Would this be acceptable?

[gundalow]

@Rylon No need to apologize, everybody is busy :)

As long as no features have been added since Ansible 2.7, then this sounds reasonable.

We are getting close to the 2.8 feature freeze https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/roadmap/ROADMAP_2_8.rst

[Rylon]

Yeah @gundalow there's no change in functionality, just a refactoring of the internals I believe. @samdoran can perhaps comment further as he did the original merge.

[samdoran]

@Rylon You can open the full backport PR and we can discuss it there to see how big the changes are. My original thought was the changes were too substantial for a backport since it changed the name of certain lookup plugin options.

We have the same issue here as in #50160 where the module in 2.7 has diverged enough from devel that it's not possible to just cherry-pick the commits from devel, and I need to make a change to the _run method to support the new 1Password CLI.

In cases like this, we generally make backport PR that is not an exact cherry-pick of an upstream patch. It is not ideal but necessary in some cases. This may be one of those cases.

[Rylon]

Sure, in this case @samdoran it’ll be completely new commits making some additional changes that are not in this PR, to fix the _run method issue, but if you’re ok with that then I can do it. 👍

[Rylon]

Ok I made the changes here: #53657

Logically it's the same functionality (achieving compatibility with 0.5.5+) but I had to remove the duplicate methods and tweak a few things. I tested it locally on Ansible 2.7.4 and functionality has been restored, without altering the interface.