- 
                Notifications
    
You must be signed in to change notification settings  - Fork 35
 
Start workspaces by shelling out to CLI #400
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
Replace the REST-API-based start flow with one that shells out to the coder CLI. Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
f8319a8    to
    59ac05c      
    Compare
  
    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.
Thank you for putting this together!  Just to test, I hard-coded --global-config and it worked great, so that is the only thing we really need to figure out.
| 
           Thanks for reviewing! I'm traveling right now but will come back to this in early December.  | 
    
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.
Looks and works great!  The only thing is that it asked me to log in again, which I think is just because of the missing await, the next line checked for session but it had not been renamed yet.  Subsequent runs are working wonderfully.
        
          
                src/storage.ts
              
                Outdated
          
        
      | await fs.stat(oldTokenPath) | ||
| const newTokenPath = this.getSessionTokenPath(label) | ||
| await fs.rename(oldTokenPath, newTokenPath) | 
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.
Not a blocker at all, just wanted to mention that stat is often an anti-pattern; because rename can also throw ENOENT and there is technically no guarantee the file still exists between the stat and rename, it is always necessary to check the error of rename anyway, so might as well do it with one call.
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.
Right, that makes sense. Removed the stat.
Co-authored-by: Asher <ash@coder.com>
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.
Thank you! After merging I will cut a new release.
Replace the REST-API-based start flow with one that shells out to the coder CLI. This is the JetBrains extension equivalent to coder/vscode-coder#400.
Replace the REST-API-based start flow with one that shells out to the coder CLI. This is the JetBrains extension equivalent to coder/vscode-coder#400.
Replace the REST-API-based start flow with one that shells out to the coder CLI.