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

passwordstore: Make compatible with shims #4780

Merged
merged 2 commits into from
Jun 15, 2022
Merged

passwordstore: Make compatible with shims #4780

merged 2 commits into from
Jun 15, 2022

Conversation

grembo
Copy link
Contributor

@grembo grembo commented Jun 5, 2022 •
edited
Loading

SUMMARY

This allows using it with scripts wrapping other password managers
like gopass. Also adds an explicit way of configuring the backend to be
used (pass and gopass).

Includes integrations tests.

Resolves #4766

ISSUE TYPE
  • Feature Pull Request
  • Test Pull Request
COMPONENT NAME

passwordstore
plugins/lookup/passwordstore

ADDITIONAL INFORMATION

See #4779 for an alternative implementation of addressing #4766.

@ansibullbot
Copy link
Collaborator

@grembo: Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibullbot ansibullbot added WIP Work in progress integration tests/integration lookup lookup plugin needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly plugins plugin (any type) tests tests labels Jun 5, 2022
@grembo grembo mentioned this pull request Jun 5, 2022
Closed
1 task
@ansibullbot ansibullbot added feature This issue/PR relates to a feature request has_issue and removed needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly labels Jun 5, 2022
@grembo grembo marked this pull request as ready for review June 5, 2022 01:57
@ansibullbot ansibullbot removed the WIP Work in progress label Jun 5, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Jun 5, 2022
felixfontein
felixfontein reviewed Jun 12, 2022
View reviewed changes
@felixfontein
Copy link
Collaborator

You are modifying the integration tests btw, not the unit tests :-)

@grembo
Copy link
Contributor Author

grembo commented Jun 12, 2022

You are modifying the integration tests btw, not the unit tests :-)

Fortunately you'll squash those commits :)

@felixfontein
Copy link
Collaborator

True, but the squashing will keep all commit messages :)

@grembo
Copy link
Contributor Author

grembo commented Jun 12, 2022

True, but the squashing will keep all commit messages :)

Well, you could edit them, otherwise the commit will be overly verbose.
Seems like the gh command line tool can do this since cli/cli@f2d23d8

@felixfontein
Copy link
Collaborator

I never do. If you want to resulting commit message to be different, you need to rebase and edit the messages yourself (or squash commits which should result in only one line).

@grembo
Copy link
Contributor Author

grembo commented Jun 12, 2022 •
edited
Loading

I never do. If you want to resulting commit message to be different, you need to rebase and edit the messages yourself (or squash commits which should result in only one line).

I can (and will) do that - I just remember from last time that you prefer to squash yourself.

@grembo grembo force-pushed the support-passwordstore-wrappers branch from b5ac8e6 to 88309b7 Compare June 12, 2022 15:49
@grembo
passwordstore: Make compatible with shims, add backend config
263ba86 
This allows using the passwordstore plugin with scripts that wrap other
password managers. Also adds an explicit configuration (`backend` in
`ini` and `passwordstore_backend` in `vars`) to set the backend to `pass`
(the default) or `gopass`, which allows using gopass as the backend
without the need of a wrapper script. Please be aware that gopass
support is currently limited, but will work for basic operations.

Includes integrations tests.

Resolves ansible-collections#4766
@grembo grembo force-pushed the support-passwordstore-wrappers branch from 88309b7 to 263ba86 Compare June 12, 2022 15:51
@grembo
Copy link
Contributor Author

grembo commented Jun 12, 2022

@felixfontein Squashed it

@felixfontein
Copy link
Collaborator

I never do. If you want to resulting commit message to be different, you need to rebase and edit the messages yourself (or squash commits which should result in only one line).

I can (and will) do that - I just remember from last time that you prefer to squash yourself.

See https://github.com/ansible-collections/community.general/blob/main/CONTRIBUTING.md#open-pull-requests, it explains why we prefer authors not to squash their commits. It also says "if not needed". Fixing individual commit messages because of errors in them is a valid reason to modify them (and also it's different from squashing everything).

@grembo
Copy link
Contributor Author

grembo commented Jun 12, 2022

So I probably oversquashed this one. It's not too bad though, as code based review comments were fairly minimal and almost all of the relevant discussion leading to it happened in #4766 anyway. Next time ¯_(ツ)_/¯

felixfontein
felixfontein reviewed Jun 12, 2022
View reviewed changes
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.

Besides two little improvements for the tests, looks good!

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 12, 2022
@ansibullbot ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 12, 2022
@felixfontein felixfontein merged commit 006f3bf into ansible-collections:main Jun 15, 2022
@patchback
Copy link

patchback bot commented Jun 15, 2022 •
edited
Loading

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/006f3bfa89f5ff95ffd83e9dbd16702f7ef9d96a/pr-4780

Backported as #4846

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

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 15, 2022
patchback bot pushed a commit that referenced this pull request Jun 15, 2022
@grembo @patchback
passwordstore: Make compatible with shims (#4780)
e1ca820 
* passwordstore: Make compatible with shims, add backend config

This allows using the passwordstore plugin with scripts that wrap other
password managers. Also adds an explicit configuration (`backend` in
`ini` and `passwordstore_backend` in `vars`) to set the backend to `pass`
(the default) or `gopass`, which allows using gopass as the backend
without the need of a wrapper script. Please be aware that gopass
support is currently limited, but will work for basic operations.

Includes integrations tests.

Resolves #4766

* Apply suggestions from code review

(cherry picked from commit 006f3bf)
@patchback patchback bot mentioned this pull request Jun 15, 2022
Merged
@felixfontein
Copy link
Collaborator

@grembo thanks for contributing this!

@felixfontein felixfontein mentioned this pull request Jun 15, 2022
Closed
felixfontein pushed a commit that referenced this pull request Jun 15, 2022
@patchback @grembo
passwordstore: Make compatible with shims (#4780) (#4846)
82a7644 
* passwordstore: Make compatible with shims, add backend config

This allows using the passwordstore plugin with scripts that wrap other
password managers. Also adds an explicit configuration (`backend` in
`ini` and `passwordstore_backend` in `vars`) to set the backend to `pass`
(the default) or `gopass`, which allows using gopass as the backend
without the need of a wrapper script. Please be aware that gopass
support is currently limited, but will work for basic operations.

Includes integrations tests.

Resolves #4766

* Apply suggestions from code review

(cherry picked from commit 006f3bf)

Co-authored-by: grembo <[email protected]>
@felixfontein felixfontein mentioned this pull request Jul 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
feature This issue/PR relates to a feature request has_issue integration tests/integration lookup lookup plugin plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

passwordstore lookup plugin gopass compatibility
3 participants
@grembo @ansibullbot @felixfontein