Skip to content

Bypass download if CLI version matches #245

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 3 commits into from
May 16, 2023
Merged

Bypass download if CLI version matches #245

merged 3 commits into from
May 16, 2023

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented May 9, 2023

This is a bit faster than waiting for a 304 but more importantly it works if the user customizes the download source to be a place that does not have the same etag support that Coder does.

The test bypasses Windows since we would need to create an executable there for the test. We could split out the exec and the parsing although I am not sure it is worth the noise. The part most likely to break between platforms is probably the exec, not the parsing?

We do indirectly test the version parsing on Windows with the test that goes against dev.coder.com.

@code-asher code-asher requested a review from johnstcn May 9, 2023 19:38
This is a bit faster than waiting for a 304 but more importantly it
works if the user customizes the download source to be a place that does
not have the same etag support that Coder does.

The test bypasses Windows since we would need to create an executable
there for the test.  We could split out the exec and the parsing
although I am not sure it is worth the noise.  The part most likely to
break between platforms is probably the exec, not the parsing?
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Change looks fine to me, if we wanted to be a bit more future-proof we would want to do a semver comparison of the versions and ensure that the versions are equivalent instead of just checking the raw string.

@code-asher code-asher force-pushed the version-check branch 3 times, most recently from 9dd62c9 to a2956e9 Compare May 10, 2023 18:52
@code-asher code-asher marked this pull request as draft May 10, 2023 18:55
@code-asher code-asher marked this pull request as ready for review May 10, 2023 22:03
@code-asher code-asher requested a review from johnstcn May 10, 2023 22:03
Comment on lines +470 to +474
null | ProcessInitException
"""echo '{"foo": true, "baz": 1}'""" | InvalidVersionException
"""echo '{"version: '""" | JsonSyntaxException
"exit 0" | InvalidVersionException
"exit 1" | InvalidExitValueException
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@code-asher code-asher merged commit abd7204 into main May 16, 2023
@code-asher code-asher deleted the version-check branch May 16, 2023 19:36
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