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.

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

Use get(..) instead of [..] for safe lookup of value (Fixes #7240) #7241

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

wkleinheerenbrink
Copy link
Contributor

@wkleinheerenbrink wkleinheerenbrink commented Sep 11, 2023

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).

SUMMARY
ISSUE TYPE
COMPONENT NAME

OnePassword

ADDITIONAL INFORMATION

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) labels Sep 11, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Can you please add a changelog fragment? Thanks.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Sep 11, 2023
@samdoran
Copy link
Contributor

It would be good to add tests for this. There is already a test written, you just need to provide new fixture data.

A new file with the output from op item get [item name] --format json (minus any sensitive data) needs to go in tests/unit/plugins/lookup/onepassword_fixtures/v2_out_04.json.

A new entry in the test matrix needs to go here. It will look something like this:

{
    # Request data from a custom field with no value
    "vault_name": "Test Vault",
    "queries": ["Dummy Login"],
    "kwargs": {
        "field": "some-omitted-property",
    },
    "expected": [""],
    "output": load_file("v2_out_04.json")
},

You can run the test from the root of the collection by running ansible-test units --docker default tests/unit/plugins/lookup/test_onepassword.py --python 3.11.

That's a lot so please feel free to ask for help. Thanks for fixing this!

…-collections#7240)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).
@samdoran
Copy link
Contributor

Looks great!

shipit

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Sep 13, 2023
@felixfontein felixfontein merged commit 1beb38c into ansible-collections:main Sep 13, 2023
@patchback
Copy link

patchback bot commented Sep 13, 2023

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/1beb38ceff80b9e1d1437cab2ba27deff6a73124/pr-7241

Backported as #7257

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 13, 2023
…7241)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).

(cherry picked from commit 1beb38c)
@patchback
Copy link

patchback bot commented Sep 13, 2023

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/1beb38ceff80b9e1d1437cab2ba27deff6a73124/pr-7241

Backported as #7258

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Sep 13, 2023
…7241)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).

(cherry picked from commit 1beb38c)
@felixfontein
Copy link
Collaborator

@wkleinheerenbrink thanks for your contribution!
@samdoran thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Sep 13, 2023
…for safe lookup of value (Fixes #7240) (#7258)

Use `get(..)` instead of [..] for safe lookup of value (Fixes #7240) (#7241)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).

(cherry picked from commit 1beb38c)

Co-authored-by: Wouter Klein Heerenbrink <wouter@fluxility.com>
felixfontein pushed a commit that referenced this pull request Sep 13, 2023
…for safe lookup of value (Fixes #7240) (#7257)

Use `get(..)` instead of [..] for safe lookup of value (Fixes #7240) (#7241)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).

(cherry picked from commit 1beb38c)

Co-authored-by: Wouter Klein Heerenbrink <wouter@fluxility.com>
@wkleinheerenbrink wkleinheerenbrink deleted the patch-1 branch September 13, 2023 06:56
etrombly pushed a commit to etrombly/community.general that referenced this pull request Oct 25, 2023
…-collections#7240) (ansible-collections#7241)

A OnePassword field item might not have a value (property) when the user has omitted it (on purpose).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug lookup lookup plugin new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OnePassword library assumes incorrectly that a field als an attribute 'value'
4 participants