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.

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

Switch to EncodedCommand to help coder.com #1774

Merged

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Feb 26, 2019

PR Summary

An alternative to fixing #1772. This will base64 encode the command that we want to run and use PowerShell's -EncodedCommand to make sure the command runs properly.

Unfortunately, -EncodedCommand assumes Unicode... in Node, we have utf16le. I want to test this on all platforms before this goes in.

Test checklist:

  • Window
  • macOS
  • Linux
  • Coder.com

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@TylerLeonhardt TylerLeonhardt changed the title WIP: Switch to EncodedCommand to help coder.com Switch to EncodedCommand to help coder.com Feb 26, 2019
@TylerLeonhardt
Copy link
Member Author

Removed WIP!

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM but did you intend to update package-lock.json with this PR (or did it sneak in)? It's not obvious to mean when we should be checking in the lock file.

@TylerLeonhardt
Copy link
Member Author

I saw that it was updated and said, "eh, I'll leave it in".

I think we should maybe make sure it's never in a PR and rely on something like:
https://greenkeeper.io

To be the only one to update it. (it does, I checked)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants