-
-
Notifications
You must be signed in to change notification settings - Fork 301
test(commands): stores gpg keyring in a temporary directory #547
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #547 +/- ## ========================================== + Coverage 98.26% 98.32% +0.05% ========================================== Files 39 39 Lines 1556 1551 -5 ========================================== - Hits 1529 1525 -4 + Misses 27 26 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I saw the tests create a bunch of keys, but it is a drawback for package’s developer and not for final user.
Why don’t use a TemporaryDirectory instead of home’s subdir?
Anyway, good improvement !
A TemporaryDirectory was my first thought as well but is there a way to enforce that the path to the sockets within the directory are shorter than 104 characters?
A TemporaryDirectory was my first thought as well, but is there a way to enforce that the path to the sockets within the directory is shorter than 104 characters?
IDK
I can do a bit of digging in a bit.
I might try something like tmp_path, get the length of current path and trim it to 104. Or maybe we could use /tmp?
Overall the PR looks great! Looking forward to merging it!
I might try something like tmp_path, get the length of current path and trim it to 104. Or maybe we could use
/tmp?Overall the PR looks great! Looking forward to merging it!
The TemporaryDirectory is already under /tmp (or whatever tmp folder depending by OS) and it is context managed.
tl;dr (too long; did not read)
I think we can count on the TemporaryDirectory solution being less than 104 characters. I'll give it another try. Thanks for your feedback.
pytest tmp_path/tmpdir
The pytest tmp_path/tmpdir on OSX is (/private/var/folders/??/???????????_?_????????????????/T/pytest-of-<username>/pytest-???/). The final couple of directories do not seem to be not customizable. This sets us over the 104 character limit, at least on OSX it did.
TemporaryDirectory:
TemporaryDirectory looks like it should work.
TemporaryDirectory is sensitive to the TMPDIR variable, among others.
- $TMPDIR on OSX:
/var/folders/??/???????????_?_????????????????/T/- Each path is sorted in this way since so that a single directory of the filesystem doesn't get over cluttered or have performance issues.
- The subdirectory is a relatively short/unique string so I think we are under 104 characters.
- Windows:
C:\Users\<username>\AppData\Local\Temp- path seems short and
gpg-agentmay not rely upon socket paths within the Windows implementation.
- Linux:
- similar situation to OSX but it is distribution specific. overwriting $TMPDIR is a workaround should the event arise.
9bab971 to
c6621fd
Compare
I think this change is ready for another round of review. Thanks for the input everyone!
Any guess as to why the coverage check failed? It seems like it should have passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @Lee-W if you agree let's merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo error on git config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this one. Once the typo has been fixed. I think we're good to merge this one
GNUPGHOME must be less than 104 characters in order for the gpg-agent to run.
c6621fd to
8058178
Compare
@woile, I think it's good now, thanks!
many thanks for your contribution! let's merge it!
Description
The unit tests were creating a bunch of keys (run tests a few times and see
gpg --list-keys).This solution creates a temporary
GNUPGHOME. Notably gpg-agent requires a socket path of less 104 characters otherwise the gpg command will fail.I am not confident this is the best approach but it is probably the easiest fix for the moment.
Is this a portable solution?
Also, I reduced the key length down to a debug level key for speed and security reasons.
Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected behavior
no more added keys.
Steps to Test This Pull Request
gpg --list-keysAdditional context