-
Notifications
You must be signed in to change notification settings - Fork 413
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: invalid $HOME is not detected #4901
fix: invalid $HOME is not detected #4901
Conversation
aa1d7ec
to
c318a6e
Compare
faaf52a
to
7a6b048
Compare
/retryBuilds |
It's not really a Windows specific issue. Any system with a misconfigured $HOME env var could have this issue. |
7a6b048
to
7e7b1a1
Compare
/runIntegrationTests |
7e7b1a1
to
7103ade
Compare
* @param {string} [path] - The path to be checked. If not provided, defaults to an empty string. | ||
* @returns {boolean} Returns true if the path is valid and exists, otherwise returns false. | ||
*/ | ||
export function isValidPath(path?: string): boolean { |
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 would never be hit in web-mode because getHomeDirectory
returns early for web-mode
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.
vscode
only provide Thenable
aka Promise
based api. To allow this in web, it will require updating all touch points with a Promise
based API.
https://github.com/microsoft/vscode/blob/main/src/vscode-dts/vscode.d.ts#L9045
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 pushed a change to introduce initUserHomeDir()
, which is a one-time routine done at startup that validates $HOME and other env vars. That allows initUserHomeDir()
to be async while getUserHomeDir()
remains "sync". As a minor perf bonus, getUserHomeDir()
now uses a cached value.
@@ -17,6 +19,7 @@ describe('sharedCredentials', function () { | |||
// Make a temp folder for all these tests | |||
// Stick some temp credentials files in there to load from | |||
tempFolder = await makeTemporaryToolkitFolder() | |||
sinon.stub(pathUtils, 'isValidPath').returns(true) |
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.
stubbing isValidPath in various tests that happen to current hit the getHomeDirectory()
codepath, means that future tests might run into the same issue.
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.
Given that in unit tests we are validating an actual path, indeed it will require stubbing in future as well. What would be your recommendation?
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.
The change that I pushed avoids this problem by only validating at startup (when initUserHomeDir()
is called).
92b8259
to
6820f30
Compare
6820f30
to
c120555
Compare
c120555
to
25199a5
Compare
25199a5
to
c60910f
Compare
3387d09
to
c0e6864
Compare
const f = resolve(envVal) | ||
fs.existsFile(f).then(r => { | ||
if (!r) { | ||
getLogger().error('$%s filepath is invalid (or is a directory): %O', envVar, f) |
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.
Error is logged asynchronously, and the value is still returned. This isn't ideal, but is less critical than the $HOME validation.
I'll followup later so that invalid AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE surface an error on startup.
c0e6864
to
8fb47eb
Compare
4fa6a29
to
39b9fc8
Compare
Problem: - `SystemUtilities.getHomeDirectory`: - does not use the cross-platform `fs.ts` module. - checks env vars every time it is called, which is a performance cost. Solution: - Move validation into a one-time `initUserHomeDir()` function which is called on startup. - Introduce `fs.getUserHomeDir()`.
39b9fc8
to
9aef201
Compare
Problem
HOME
orUSERPROFILE
orHOMEPATH
AWS_SHARED_CREDENTIALS_FILE
orAWS_CONFIG_FILE
are unable to login into Amazon Q and AWS Toolkits extensions.Solution
os.homedir()
.License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.