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

Unify terms and UI between 1Password lookups and facts module #45427

Merged
merged 8 commits into from
Sep 21, 2018
Merged

Unify terms and UI between 1Password lookups and facts module #45427

merged 8 commits into from
Sep 21, 2018

Conversation

samdoran
Copy link
Contributor

SUMMARY

Unify the terms and interface used for logging in to 1Password between the facts module and lookup plugins.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

onepassword.py
onepassword_raw.py
onepassword_facts.py

ANSIBLE VERSION
2.7

@samdoran
Unify login behavior between 1Password lookup plugins and module
a34d5b6 
- Use the same names for all credential aspects
- Only require the minimal amount of information for each
- Add more examples
@samdoran
Change parameter terms
aef5dcf 
- use terms in line with 1Password documentation.
- update examples
- update tests
@ansibot
Copy link
Contributor

ansibot commented Sep 10, 2018

cc @Rylon
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Sep 10, 2018
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Sep 10, 2018
@samdoran
Improve error messages in lookup plugin
1108bc2
@samdoran
Unify onepassword_facts with lookup plugins
a6f6e3e 
- use same methods and logic for signing in or reusing existing session
- unify terms with lookup plugins
@ansibot
Copy link
Contributor

ansibot commented Sep 10, 2018

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:148:0: ImportError: No module named errors

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:148:0: ImportError: No module named errors

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:148:0: ImportError: No module named 'ansible.errors'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:148:0: ModuleNotFoundError: No module named 'ansible.errors'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:148:0: ModuleNotFoundError: No module named 'ansible.errors'

The test ansible-test sanity --test use-argspec-type-path [explain] failed with 1 error:

lib/ansible/modules/identity/onepassword_facts.py:157:41: use argspec type="path" instead of type="str" to avoid use of `expanduser`

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 10, 2018
@Rylon
Copy link
Contributor

Rylon commented Sep 11, 2018

Quick question about error messages @samdoran in the module docs I added recommendation for using the module with no_log: true, but the issue there is that also hides error messages raised by the module, such as "item not found in 1Password".

Can you think of a better way to protect from accidentally logging secrets, whilst allowing error messages to come through?

@samdoran
Change rc test for determing login
fce08bf 
An rc other than 1 can be returned when a current login session does not exist.
@samdoran
Create AnsibleModuleError class
855bf69 
ansible.errors is not available to modules, so create an AnsibleModuleError class within the module

Do not user os.path.expanduser since this is already done by virtue of the type being "path" in the argument spec.
@samdoran
Copy link
Contributor Author

Unfortunately, no. We don't currently have a mechanism for marking facts as sensitive/no_log, only input parameters (which I added to master_password and secret_key, so that's something 😃).

We should probably add documentation that these items are set as Ansible facts, which are subject to caching.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 11, 2018
@samdoran
Copy link
Contributor Author

But normally the returned data is not logged to the console, so running without -v will still show errors and hide returned facts.

@samdoran samdoran changed the title [WIP]: Unify terms and UI between 1Password lookups and facts module Unify terms and UI between 1Password lookups and facts module Sep 12, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Sep 12, 2018
@samdoran
Copy link
Contributor Author

@Rylon Have you had a chance to look this over? What do you think?

@Rylon
Copy link
Contributor

Rylon commented Sep 20, 2018

Hi @samdoran I just had a look through now, and it all looks good to me! One additional thing I just thought of though, I notice you removed the 1Password CLI version from the docs (v0.5.1) and I remembered I had added that there to give users a clue which version we had tested with. Perhaps we can do something better here. This would be just in case the CLI 'signature' changes substantially between versions and we're unable to parse it properly.

Otherwise, looks good to me!

@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 Sep 20, 2018
@samdoran
Copy link
Contributor Author

@Rylon Thank you for the feedback. I did remove the version number from the docs since those things don't age well. But it's probably a good idea to leave it in there. I'll put it back.

@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 Sep 21, 2018
Rylon
Rylon approved these changes Sep 21, 2018
View reviewed changes
Copy link
Contributor

@Rylon Rylon left a comment

Choose a reason for hiding this comment

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

Nice, looks good!

@samdoran samdoran merged commit b608543 into ansible:devel Sep 21, 2018
@samdoran samdoran deleted the 1password-unification branch September 21, 2018 18:26
@samdoran samdoran mentioned this pull request Jan 14, 2019
Merged
@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

@Rylon Rylon Rylon 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. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samdoran @ansibot @Rylon @bcoca