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

Allow disabling keyring #8910

Merged

Conversation

Popkornium18
Copy link
Contributor

@Popkornium18 Popkornium18 commented Jan 26, 2024

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
Copy link

github-actions bot commented Feb 2, 2024

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

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?

tests/utils/test_password_manager.py Outdated Show resolved Hide resolved
src/poetry/config/config.py Show resolved Hide resolved
src/poetry/utils/password_manager.py Outdated Show resolved Hide resolved
docs/configuration.md Show resolved Hide resolved
docs/faq.md Outdated Show resolved Hide resolved
docs/repositories.md Outdated Show resolved Hide resolved
@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

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?

@radoering
Copy link
Member

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
Copy link
Contributor Author

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.

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.

tests/utils/test_password_manager.py Show resolved Hide resolved
@radoering radoering merged commit 4433307 into python-poetry:master Feb 5, 2024
33 checks passed
@Popkornium18 Popkornium18 deleted the allow_disabling_keyring branch February 5, 2024 22:39
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.
Labels
impact/docs Contains or requires documentation changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants