Skip to content

fix: keep binary in same place, update if needed #33

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

Merged
merged 11 commits into from
Jan 20, 2023

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Jan 17, 2023

Description

@deansheather filed a bug related to the Windows Firewall prompt coming up with every new binary (i.e. every time we download coder to an updated version).

After some initial testing, it appears keeping the binary in the same place will make the prompt only appear on the first time.

Previous implementation kept the version in the path.

We decided to refactor things to add support for ETag on the server and then use those changes on the client (aka in the extension).

This means now the client will tell the server "Here's the version I have. What should I do?" and send the appropriate headers.

Checklist

  • if needed, change path/name of location to not include version
  • if the binary exists, SHA1 it and use it as the If-None-Match header in your request
  • Server Response changes
    • on 200 OK: move the binary on top of the old one
    • on 304 Not Modified: delete the temp file and use the old binary

Testing Plan

TBD

Fixes #23

@jsjoeio jsjoeio changed the title wip fix: keep binary in same place, update if needed Jan 17, 2023
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-binary-windows branch from 4caf765 to d06c873 Compare January 17, 2023 21:04
@jsjoeio jsjoeio requested a review from kylecarbs January 17, 2023 21:07
@jsjoeio jsjoeio self-assigned this Jan 17, 2023
@jsjoeio jsjoeio requested a review from deansheather January 17, 2023 21:07
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 17, 2023

What's the easiest way to test these changes locally @kylecarbs ?

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

oops meant to request changes

@jsjoeio jsjoeio requested a review from deansheather January 17, 2023 22:19
@jsjoeio jsjoeio marked this pull request as ready for review January 18, 2023 15:49
Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

the logic seems OK but I haven't tested it

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 18, 2023

@deansheather re: testing

Let me make these changes, build to a vsix then send to you via Slack to test

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 18, 2023

I tested locally on macOS and something is off

image

Seems like it's trying to use the binary before it's done renaming it?

This removes the version from the binary path to ensure that the path
always remains the same on OS's.

This way, tools like Windows Firewall and macOS Firewill will only
prompt for a security check on the first install but not subsequent
installs.
This makes significant changes to the `fetchBinary` logic.

First, it refactors a couple pieces of logic into methods on the
`Storage` class to make the code more readable.

Then it modifies the flow to first check if the binary is outdated. If
it is, then it downloads the latest version.
@jsjoeio jsjoeio force-pushed the jsjoeio/fix-binary-windows branch from 36da848 to 369a823 Compare January 18, 2023 16:32
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 18, 2023

I'm going to try rebasing on main and then rebuilding and testing again.

EDIT: same issue

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 18, 2023

I figured it out - I was trying to rm the file after mving and so it was failing because it couldn't remove a file that doesn't exist 🤦🏼‍♂️

This will build a `.vsix` file locally marked as a pre-release which is
handy for testing.
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 18, 2023

Tested locally and seems to work 👍🏼

image

@jsjoeio jsjoeio requested a review from deansheather January 18, 2023 18:02
This makes a couple new changes:
- if 200, move old binPath to binPath.old-
- try removing .old binary
- on start, cleanUpOldBinaries
- catch all potential errors from `fs`
- only rename binPath to old right before replacing with tempFile
This generates the Etag correctly using a readableStream and the appends
it to the headers. This also adds some logging around etag, binPath and
status code.
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jan 20, 2023

@deansheather ready for a new review!

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

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

Looks good and works on Windows for me. The logic seems OK but I'm not a big typescript guy so someone else should approve too

jsjoeio and others added 2 commits January 20, 2023 11:01
Co-authored-by: Dean Sheather <[email protected]>
Co-authored-by: Dean Sheather <[email protected]>
Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

Really appreciate the comments you left!

@jsjoeio jsjoeio merged commit ec4aa92 into main Jan 20, 2023
@jsjoeio jsjoeio deleted the jsjoeio/fix-binary-windows branch January 20, 2023 22:44
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.

Windows Firewall prompt on every new binary
3 participants