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

onepassword_doc: fix 1Password Connect support #9625

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

It seems that when #7116 and #7490 were merged, onepassword_doc.py ended up with the CLI parameters from OnePassCLIv2.get_raw() before 1Password Connect support was merged for onepassword.py. I'm not 100% sure all these parameters work with docs as well, but I think it's better to move adding these parameters to a separate function so it can be used from other derivatives of OnePassCLIv2.

@samdoran would be great if you could take a look :)

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

onepassword_doc

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch labels Jan 25, 2025
@ansibullbot
Copy link
Collaborator

cc @azenk
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug lookup lookup plugin plugins plugin (any type) labels Jan 25, 2025
@samdoran
Copy link
Contributor

This is a good short term solution. Longer term, it would be nice to have a better and cleaner interface for specifying just the particular command that needs running instead of each plugin having to override get_raw.

Maybe it's as simple as making get_raw accept args. 🤔

I like the model of plugins for different types calling get_raw with different args and handling the processing of that output in that particular plugin. That seems to be a good separation of concerns.

@@ -582,6 +580,10 @@ def get_raw(self, item_id, vault=None, token=None):

return self._run(args)

def get_raw(self, item_id, vault=None, token=None):
args = ["item", "get", item_id, "--format", "json"]
return self._add_parameters_and_run(args, vault=vault, token=token)
Copy link
Contributor

@samdoran samdoran Jan 27, 2025

Choose a reason for hiding this comment

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

Another approach could be moving this to a new _run() method on the OnePassCLIv2 class.

@felixfontein
Copy link
Collaborator Author

This is a good short term solution. Longer term, it would be nice to have a better and cleaner interface for specifying just the particular command that needs running instead of each plugin having to override get_raw.

Indeed. This is only meant as a short-term solution that can be backported without a larger refactoring. There's more work to do - the shared code should also better be moved to plugin_utils or module_utils, etc.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch backport-10 Automatically create a backport for the stable-10 branch bug This issue/PR relates to a bug check-before-release PR will be looked at again shortly before release and merged if possible. lookup lookup plugin plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants