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

onepassword_ssh_key: avoid inheriting from OnePassCLIv2 #9633

Merged
merged 2 commits into from
Jan 27, 2025
Merged

onepassword_ssh_key: avoid inheriting from OnePassCLIv2 #9633

merged 2 commits into from
Jan 27, 2025

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Follow-up to #9580.

This way it's not necessary to inherit from from OnePassCLIv2.

@mohammedbabelly20 what do you think?

ISSUE TYPE
  • Refactoring Pull Request
COMPONENT NAME

onepassword_ssh_key lookup

mohammedbabelly20
mohammedbabelly20 reviewed Jan 26, 2025
View reviewed changes
Copy link
Contributor

@mohammedbabelly20 mohammedbabelly20 left a comment

Choose a reason for hiding this comment

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

I don't really like that the LookupModule contains the get_ssh_key method ))
I still think that keeping OnePassCLIv2SSHKey is cleaner.

@ansibullbot
Copy link
Collaborator

cc @samdoran
click here for bot help

@ansibullbot ansibullbot added lookup lookup plugin plugins plugin (any type) labels Jan 26, 2025
@felixfontein
Copy link
Collaborator Author

I don't really like that the LookupModule contains the get_ssh_key method ))

Why? That's where lookup plugins usually have their code (and where it should be).

I still think that keeping OnePassCLIv2SSHKey is cleaner.

Keeping it there increases interdependency and makes it harder to add new features to these plugins.

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

Why? That's where lookup plugins usually have their code (and where it should be).

After reviewing other modules, I agree with you, it makes sense to follow the existing pattern for lookup plugins.

mohammedbabelly20
mohammedbabelly20 approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor

@mohammedbabelly20 mohammedbabelly20 left a comment

Choose a reason for hiding this comment

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

LGTM

@felixfontein felixfontein merged commit 8749da7 into ansible-collections:main Jan 27, 2025
138 checks passed
@patchback Patchback
Copy link

patchback bot commented Jan 27, 2025 •
edited
Loading

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/8749da775684127ae8ec13c65afe94da666d5fd1/pr-9633

Backported as #9639

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

@felixfontein felixfontein deleted the onepassword_ssh_key branch January 27, 2025 06:24
@felixfontein
Copy link
Collaborator Author

@mohammedbabelly20 thanks for reviewing!

patchback bot pushed a commit that referenced this pull request Jan 27, 2025
@felixfontein @patchback
onepassword_ssh_key: avoid inheriting from OnePassCLIv2 (#9633)
b26f794 
* Avoid inheriting from OnePassCLIv2.

* Add changelog fragment.

(cherry picked from commit 8749da7)
@patchback patchback bot mentioned this pull request Jan 27, 2025
Merged
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 27, 2025
felixfontein added a commit that referenced this pull request Jan 27, 2025
@patchback @felixfontein
[PR #9633/8749da77 backport][stable-10] onepassword_ssh_key: avoid in…
0331798 
…heriting from OnePassCLIv2 (#9639)

onepassword_ssh_key: avoid inheriting from OnePassCLIv2 (#9633)

* Avoid inheriting from OnePassCLIv2.

* Add changelog fragment.

(cherry picked from commit 8749da7)

Co-authored-by: Felix Fontein <[email protected]>
Massl123 pushed a commit to Massl123/community.general that referenced this pull request Feb 7, 2025
@felixfontein @Massl123
onepassword_ssh_key: avoid inheriting from OnePassCLIv2 (ansible-coll…
5310655 
…ections#9633)

* Avoid inheriting from OnePassCLIv2.

* Add changelog fragment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohammedbabelly20 mohammedbabelly20 mohammedbabelly20 approved these changes

Assignees
No one assigned
Labels
backport-10 Automatically create a backport for the stable-10 branch lookup lookup plugin plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@felixfontein @ansibullbot @mohammedbabelly20