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

fix: fix op cli always generating passwords #127

Merged
merged 2 commits into from
Jan 2, 2024

Conversation

josh-burton
Copy link
Contributor

@josh-burton josh-burton commented Dec 14, 2023

When invoking the op cli, if the onepassword item had no password recipe, generate-password would always be appended to the cli args and any password value would be overwritten by a generated password.

Note I am very new to Go so please point me in the right direction if needed :)

As part of this fix I have extracted the logic which constructs the generate-password cli arg into a function in utils, and implemented a test around it.

Fixes #128

When invoking the op cli, if the onepassword item had no password recipe, generate-password would always be appended to the cli args and any password value would be overwritten by a generated password
Copy link
Member

@edif2008 edif2008 left a comment

Choose a reason for hiding this comment

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

Thank you so much for providing a fix for the bug mentioned in #128. I really appreciate it. 💪🏻

Functional review: ✅

Validated that this code will no longer generate passwords when creating an item and the user provided a custom value to the password.

Code review: ✅

Code looks clean and straight forward. I really appreciate you extracting the functionality for getting the password recipe for an item, as well as writing tests for that new functionality.

I left one nitpick that would simplify some code, however the current implementation is good already.

Other notes

Could you add in the PR description that it resolves issue #128? In this way, when we merge this PR, the issue will close automatically. 😄

Adding this line should suffice:

Resolves: #128 

onepassword/cli/utils.go Outdated Show resolved Hide resolved
@josh-burton
Copy link
Contributor Author

@edif2008 thanks for the review :) I've updated the PR description and applied your suggestion.

@edif2008 edif2008 merged commit c51f434 into 1Password:main Jan 2, 2024
2 checks passed
@josh-burton josh-burton deleted the op-cli-default-password-recipe branch January 2, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform cannot create items with the password we provide in the code
2 participants