-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Fix/issue #286 #365
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/issue #286 #365
Conversation
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
We should handle code-server --password secretpassword (no =) as well. code-server -P is also supposed to work I believe.
@code-asher is right. According to code-server --help, this is the format:
-P, --password <value> Specify a password for authentication.
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.
My bad, I didn't include -P in my example but we'll need that as well. No need to check for -P= though since = isn't used with short options.
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.
Another unfortunate edge case comes to mind:
code-server --data-dir -- password pass
That is, -- is only interpreted as the end of options if it isn't the argument for another option.
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.
It's possible we should use an environment variable and/or configuration file instead of a command line argument for the password. Or possibly a prompt, although I'm not really keen on that solution.
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 believe we have another issue for that #349
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.
Instead of searching for the flag, we should just search for the password itself so this code is future-proof and simple.
We should probably use an environment variable and deprecate the usage via CLI.
Using an ENV password will make this PR obsolete. I'll go ahead and close this PR.
Problem: CLI Password argument is visible in
ps -axIssue: #286
Changes: Adds script to change the process display name.
Test Environment:
OS: Mac OS High Sierra (10.13.6)
Browser: Google Chrome 72.0.3626.109 (Official Build) (64-bit)
(Result: Pass! Compile was successful and everything functions as expected)