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

Fix get-cli.sh script to point to correct windows assest #286

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

BMartinos
Copy link
Collaborator

The get-cli.sh script was pointing to an incorrect windows assets, and was never updating/downloading the windows version correctly.

https://github.com/openhie/instant-v2/releases/
https://github.com/openhie/instant-v2/releases/download/2.3.1/instant-win.exe

@arran-standish
Copy link
Collaborator

It might be better to output a warning to instead consider using WSL if they try download the windows binary. Since the majority of images used in platform assume a Linux base and don't actually have a valid windows image it can use. Might give the wrong assumption.

@arran-standish
Copy link
Collaborator

Also if you are changing the get-cli.sh file it would be nice opportunity to update the root url for all links to https://github.com/openhie/instant-v2/ instead. It does have a redirect rule currently so it is not an issue but would make it more consistent.

@BMartinos
Copy link
Collaborator Author

BMartinos commented Apr 24, 2024

Also if you are changing the get-cli.sh file it would be nice opportunity to update the root url for all links to https://github.com/openhie/instant-v2/ instead. It does have a redirect rule currently so it is not an issue but would make it more consistent.

Updated in bf98379

@BMartinos
Copy link
Collaborator Author

For the WSL related comment, Good suggestion, Ill leave that enhancement to the Platform team to update. If there are packages which currently dont run on Windows base image, it might be better to not have a windows based installer, but there might be reason its still needed which im not familiar with

@rcrichton
Copy link
Member

On the windows issue. I think docker desktop supports WSL so a windows binary still might make sense. We would need someone to try this out however. At one stage we were able to get it to work but I think we need to revise.

@rcrichton
Copy link
Member

Also, I think instead of fetching the cli and have it sitting in the working directory, people should be installing instant on their machine like any other CLI as described ehre - https://jembi.gitbook.io/instant-v2/getting-started/quick-start

Maybe we can get rid of this script all together.

@BMartinos
Copy link
Collaborator Author

Also, I think instead of fetching the cli and have it sitting in the working directory, people should be installing instant on their machine like any other CLI as described ehre - https://jembi.gitbook.io/instant-v2/getting-started/quick-start

Maybe we can get rid of this script all together.

Also a good idea 👍

Maybe taking this further, instead of removing the script, repurpose it to instead of installing the binary in the directory root, but to rather have the script install/update the the binary (of choice) for global CLI usage, as described in the quick start steps

@BMartinos BMartinos merged commit b2752e7 into main Apr 25, 2024
@BMartinos BMartinos deleted the fix-get-cli-for-windows-issue branch April 25, 2024 10: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.

3 participants