-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix(cli): empty ~/.codemod same as non-existent for credentials #1398
fix(cli): empty ~/.codemod same as non-existent for credentials #1398
Conversation
@spirulence is attempting to deploy a commit to the Codemod Team on Vercel. A member of the Team first needs to authorize it. |
346f241
to
d2bf003
Compare
codemod login
c7042a7
to
ce6c6cf
Compare
I receive a clean test run with this new test suite:
|
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.
Some self-review.
vol.fromJSON({ "": "" }, "/home/codemod-test/.codemod"); | ||
|
||
const storage = new CredentialsStorage(); | ||
expect(storage.get(testAccount)).rejects.toThrow(); |
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.
Would not be hard to expect the specific ENOTDIR error to be thrown. Might do.
|
||
describe("when .codemod directory does not exist", () => { | ||
beforeEach(() => { | ||
vol.reset(); |
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.
This before each block is repeated on each test. Would be happy to move to enclosing scope.
vi.mock("node:fs/promises", () => fs.promises); | ||
|
||
// Use a mock homedir in order to reliably have a memfs home directory | ||
vi.mock("node:os", () => ({ homedir: () => "/home/codemod-test/" })); |
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.
If there is a better way to enforce a homedirectory than by mocking an implementation internal, I'm down to do that instead.
import { | ||
CredentialsStorage, | ||
CredentialsStorageType, | ||
} from "../src/credentials-storage.js"; |
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.
Is a relative import the expected way to pull in the code under test?
); | ||
} catch (error) { | ||
// If the directory does not exist, treat it as "no results" | ||
if ((error as NodeJS.ErrnoException).code === "ENOENT") { |
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 also need to test what happens here if readdir
throws an error that is not a NodeJS.ErrnoException
or does not have a .code
attribute
closing in favor of #1401 |
📚 Description
When a user attempts to login to the codemod platform using the CLI, and doesn't have the right binaries installed for
keytar
to work as intended, it's supposed to print an error like the following to help users install the right packages, or allow them to proceed with plaintext password storage if they want to live more adventurously:However, first time installers of the
codemod
cli may run into this issue if they do not have a~/.codemod
directory in their home path:Because the login command will create this directory when successfully setting authentication credentials, the more proper behavior would be to treat a non-existent directory as an empty one.
🔗 Linked Issue
Fixes #1397
🧪 Test Plan
On the latest
main
, in addition to npmlatest
, runningcodemod login
locally has different results if the ~/.codemod directory is missing or simply empty.Build first:
npx turbo build
Then run the following:
Using the version in this branch, the behavior is consistent:
The change is accomplished by breaking out the logic that reads the directory into it's own function, which returns an empty list of credentials to the caller if the directory is empty or if it doesn't exist. Unit tests using
memfs
are provided to test this functionality.📄 Documentation to Update
Unaware of any documentation to update.