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

Make keyring lookup more flexible #267

Merged
merged 11 commits into from
Nov 9, 2020
Merged

Make keyring lookup more flexible #267

merged 11 commits into from
Nov 9, 2020

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Nov 6, 2020

Closes #263.

@jwodder jwodder added the patch Increment the patch version when merged label Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020 •
edited
Loading

Codecov Report

Merging #267 (c74b87c) into master (f424a31) will increase coverage by 1.26%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   81.04%   82.30%   +1.26%     
==========================================
  Files          52       54       +2     
  Lines        4362     4589     +227     
==========================================
+ Hits         3535     3777     +242     
+ Misses        827      812      -15
Flag Coverage Δ
unittests 82.30% <100.00%> (+1.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dandi/girder.py 87.88% <100.00%> (+5.55%) ⬆️
dandi/support/tests/test_ui.py 100.00% <100.00%> (ø)
dandi/support/ui.py 100.00% <100.00%> (ø)
dandi/tests/test_girder.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f424a31...c74b87c. Read the comment docs.

yarikoptic
yarikoptic previously requested changes Nov 6, 2020
View reviewed changes
Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Primary concern: need of test(s).

dandi/girder.py Outdated
from keyring.backend import get_all_keyring
from keyring.errors import InitError
from keyring.util.platform_ import config_root
from keyrings.alt.file import EncryptedKeyring
Copy link
Member

Choose a reason for hiding this comment

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

why not to import them all on top?

Copy link
Member

Choose a reason for hiding this comment

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

or may be keep them here but we do need some unit (via mockups) and/or integration (do specify PYTHON_KEYRING_BACKEND pointing to some pre-crafted file) test to verify this logic. It would also be sad if we miss the moment when keyring introduces some regression etc and we do not notice since no coverage here

setup.cfg Outdated Show resolved Hide resolved
dandi/girder.py Outdated Show resolved Hide resolved
dandi/girder.py Show resolved Hide resolved
@jwodder
Copy link
Member Author

jwodder commented Nov 6, 2020

@yarikoptic Tests added.

yarikoptic
yarikoptic reviewed Nov 7, 2020
View reviewed changes
setup.cfg
@@ -73,6 +74,7 @@ style =
pre-commit
test =
coverage
pyfakefs ~= 4.0
Copy link
Member

Choose a reason for hiding this comment

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

note to myself -- provides fs fixture.

@yarikoptic yarikoptic mentioned this pull request Nov 9, 2020
Closed
@yarikoptic
Copy link
Member

Seems to work nicely. One gotcha to keep in mind (add to our handbook) is that it would require to set the password for the encrypted keyring:

2020-11-09 11:26:40,026 [    INFO] Configuring /home/yoh/.local/share/python_keyring/keyringrc.cfg to use EncryptedKeyring as default backend
Please provide API Key (created/found in My Account/API keys in Girder) for dandi: fu1oj0NC2QCbSkhCe6V2cTDalwpJOfaPiiVGGs2P
Please set a password for your new keyring: 
Please confirm the password: 
Error: blank passwords aren't allowed.
Please set a password for your new keyring: 
Please confirm the password:

initiated dandi/handbook#6

@yarikoptic yarikoptic merged commit 7e83e96 into master Nov 9, 2020
@yarikoptic yarikoptic deleted the gh-263 branch November 9, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yarikoptic yarikoptic yarikoptic left review comments

Assignees
No one assigned
Labels
patch Increment the patch version when merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Establish some local keyring store if there is no default one available
2 participants
@jwodder @yarikoptic