replace lastpass cli with 1password cli #39
Conversation
There was a problem hiding this comment.
I feel like the right call here is to add support for 1password and throw some deprecated warnings around lpass.
I know that Elijah Howells uses the CLI version of records-mover and thus is still reliant on lpass to retrieve core secrets. I had to install the 1password CLI myself so I'm pretty sure other people will have to as well. There's certain to be other people that use records-mover in a similar way.
There was a problem hiding this comment.
This is looking great. I've left some notes on your comments and the tests that were deleted. Basically, as long as we're going the quick and dirty route of swapping out the guts without relabeling everything, the disclaimer comment needs to be more strongly worded and harder to miss. 😉
.python-version
Outdated
| @@ -1 +1 @@ | |||
| db_facts-3.8.4 | |||
| db_facts-5.0.0 | |||
There was a problem hiding this comment.
This looks like it normally refers to the Python version. Should that be updated here?
There was a problem hiding this comment.
Great catch! Yes, this change should be reverted.
This refers to the python version selected for the local development python virtual environment. The venv gets created in deps.sh.
FWIW, circleci ignores this file so it doesn't affect any of those pipelines, only local dev.
There was a problem hiding this comment.
oh woops yeah i messed this file up mb
| @patch('db_facts.lpass.check_output') | ||
| def test_lpass_field_url(self, mock_check_output): | ||
| mock_check_output.return_value = "fakeurl\n".encode("utf-8") | ||
| out = lpass.lpass_field('my_name', 'url') | ||
| mock_check_output.\ | ||
| assert_called_with(['lpass', 'show', '--url', 'my_name']) | ||
| assert out == "fakeurl" |
There was a problem hiding this comment.
same deal as with the Notes field test function -- I'd prefer for this to throw an error rather than be deleted entirely
| # This used to use the lastpass-cli to pull credentials. But we've moved | ||
| # from lastpass to 1password. This command retrieves the fields in the | ||
| # same format from 1password instead. | ||
|
|
There was a problem hiding this comment.
Can you make this comment even more explicit? I'd like it to make clear that ALL functions referencing LastPass/lpass are being redirected to pull data from 1Password instead. The names have not been changed in the interest of time. This needs to be an unmissable comment for anyone working on this in the future, so you can go wild with all caps.
| @@ -4,61 +4,47 @@ | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Please add an impossible-to-miss comment here with the same disclaimer from lpass.py
Ngl throwing some warnings in the code a couple months ago would have been the more elegant solution! But as you mentioned in the slack thread, there have been ample warnings about the deprecation of LastPass elsewhere so this isn't a surprise to anyone. Right now, just getting this to work with 1Password is the top priority. I do agree that it's confusing to have things improperly named, and I'd love to fix that in the next release. Some of the functions/variables referring to LastPass might be more maintainable if they referred to the broader category "PasswordManager" instead (rather than replacing with "1Password"). Let's come back to names in a couple weeks. Everyone is going to have to install the 1Password CLI regardless of our work on this library, so that shouldn't be a factor here. |
|
@monikered Is having 1password CLI on everyones' laptop a ticket for alectronica to work on? |
Yes! They're also looking at options for distributing these updates. |
Great! :) Is there already a ticket for that alectronica and if so can we link it here? Also, should we create a jira ticket for this work in addition to the git issue you opened? I don't have an opinion either way but I wanted to ask the question in case there were additional processes we needed to follow. :) |
|
The new db-facts homebrew formula can list the 1password cli as a dependency so it would auto install when updating db-facts via homebrew |
No description provided.