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

passwordstore lookup: allow to pass options as lookup options #5444

Merged
merged 2 commits into from
Nov 2, 2022

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Something which should have been done a long time ago: allow to pass in options as lookup options instead of insisting that they have to be parsed from the terms.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

passwordstore lookup plugin

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request integration tests/integration lookup lookup plugin plugins plugin (any type) tests tests labels Oct 30, 2022
@github-actions
Copy link

github-actions bot commented Oct 30, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 30, 2022
@felixfontein
Copy link
Collaborator Author

CC @baierjan @bergmannf @grembo @phaer @TheLastProject since you contributed to this plugin before. Would be glad if you could review this / try this out!

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 30, 2022
Copy link
Contributor

@baierjan baierjan left a comment

Choose a reason for hiding this comment

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

That is quite a big change, but it definitely looks cleaner that way. Fine by me. 👍🏻

Copy link
Contributor

@TheLastProject TheLastProject left a comment

Choose a reason for hiding this comment

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

I agree this is generally an improvement and improves consistency with other modules. But it will hurt people a bit as they'll have to refactor their Ansible code. Definitely needs a major release and needs to be marked as a major change imho.

Also note I haven't tested this, just looking at how the code looks to me.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 31, 2022
@felixfontein
Copy link
Collaborator Author

But it will hurt people a bit as they'll have to refactor their Ansible code.

But why? Everything that worked so far still works. (Except if you passed lookup parameters and expected them to not work.)

This PR only allows a new (better) way to provide options. The old way still works (it should get deprecated eventually, but definitely not now).

@TheLastProject
Copy link
Contributor

Nevermind them, misunderstood the code in that case :)

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 31, 2022
@felixfontein felixfontein merged commit faf4ec7 into ansible-collections:main Nov 2, 2022
@felixfontein felixfontein deleted the lookup2 branch November 2, 2022 19:17
@felixfontein
Copy link
Collaborator Author

@baierjan @TheLastProject thanks a lot for reviewing this!

rekup pushed a commit to rekup/community.general that referenced this pull request Nov 3, 2022
russoz pushed a commit to russoz-ansible/community.general that referenced this pull request Nov 5, 2022
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
bratwurzt pushed a commit to bratwurzt/community.general that referenced this pull request Nov 7, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request integration tests/integration lookup lookup plugin plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants