Skip to content

Comments

Allow disabling keyring#8910

Merged
radoering merged 2 commits intopython-poetry:masterfrom
Popkornium18:allow_disabling_keyring
Feb 5, 2024
Merged

Allow disabling keyring#8910
radoering merged 2 commits intopython-poetry:masterfrom
Popkornium18:allow_disabling_keyring

Conversation

@Popkornium18
Copy link
Contributor

@Popkornium18 Popkornium18 commented Jan 26, 2024 •
edited
Loading

This PR allows disabling the keyring completely. I have found myself often in situations where the keyring caused a lot of trouble. Checking whether the keyring was available would cause poetry to hang indefinetely. Rather than fixing broken keyring setups on various Linux systems I would much rather just disable the keyring altogether and accept the risk of plaintext credentials.

  • Added tests for changed code.
  • Updated documentation for changed code.

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch 5 times, most recently from 030aaaa to becfbd9 Compare January 30, 2024 09:03
@radoering radoering added the impact/docs Contains or requires documentation changes label Feb 2, 2024
@github-actions
Copy link

github-actions bot commented Feb 2, 2024 •
edited
Loading

Deploy preview for website ready!

✅ Preview
https://website-owlh0kvmj-python-poetry.vercel.app

Built with commit de19d4a.
This pull request is being automatically deployed with vercel-action

radoering
radoering requested changes Feb 2, 2024
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Looks good in general. I wonder if we should add some kind of tests in password_manager to check that keyring is not used when calling certain methods or if this does not add much value?

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch 2 times, most recently from b8ce740 to 171c69a Compare February 3, 2024 13:49
@Popkornium18
Copy link
Contributor Author

Popkornium18 commented Feb 3, 2024

I made all the changes you requestet @radoering.

Looks good in general. I wonder if we should add some kind of tests in password_manager to check that keyring is not used when calling certain methods or if this does not add much value?

What certain methods would this be?

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch from 171c69a to e997e39 Compare February 3, 2024 13:53
@radoering
Copy link
Member

radoering commented Feb 3, 2024

It's probably just like "when use_keyring is not set, then none of the methods of PasswordManager tries to access keyring. I can imagine a test that replaces keyring with a mock, then calls each method and afterwards checks that the mock was never called.

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch from e997e39 to 33cf92f Compare February 3, 2024 17:32
@Popkornium18
Copy link
Contributor Author

Popkornium18 commented Feb 3, 2024

It's probably just like "when use_keyring is not set, then none of the methods of PasswordManager tries to access keyring. I can imagine a test that replaces keyring with a mock, then calls each method and afterwards checks that the mock was never called.

I implemented such a test and actually found an issue with it, so I guess it is necessary. Not sure what you think of the attempt to detect code-changes that were not reflected in the test. Looks weird, but I think it might be worth it. Better to have one more place to make a change than a bug.

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch from 33cf92f to baa8ed2 Compare February 3, 2024 17:45
radoering
radoering requested changes Feb 4, 2024
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

I implemented such a test and actually found an issue with it, so I guess it is necessary.

Nice.

Not sure what you think of the attempt to detect code-changes that were not reflected in the test. Looks weird, but I think it might be worth it. Better to have one more place to make a change than a bug.

I completely agree. I don't think it's an issue that the unit test of password_manager has to be adapted if password_manager is changed.

The test looks good to me. Just a small suggestion to make the idea behind the test more clear.

@Popkornium18 Popkornium18 force-pushed the allow_disabling_keyring branch from baa8ed2 to 310136b Compare February 5, 2024 12:11
@radoering radoering force-pushed the allow_disabling_keyring branch from 310136b to de19d4a Compare February 5, 2024 15:50
@radoering radoering merged commit 4433307 into python-poetry:master Feb 5, 2024
@Popkornium18 Popkornium18 deleted the allow_disabling_keyring branch February 5, 2024 22:39
This was referenced Feb 17, 2024
Merged
Closed
@github-actions
Copy link

github-actions bot commented Mar 6, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radoering radoering radoering approved these changes

Assignees

No one assigned

Labels

impact/docs Contains or requires documentation changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Popkornium18 @radoering