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(install): for MacOS compatibility, remove usage of long parameter for cut and tr #1002

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

curzolapierre
Copy link
Member

@curzolapierre curzolapierre commented Sep 7, 2023

- [ ] Add a changelog entry in the section "To Be Released" of CHANGELOG.md

Fixes #1001

@curzolapierre curzolapierre self-assigned this Sep 7, 2023
@curzolapierre curzolapierre marked this pull request as ready for review September 7, 2023 09:49
Copy link
Member

@EtienneM EtienneM left a comment

Choose a reason for hiding this comment

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

A non blocking question, so approving it

@@ -110,7 +118,7 @@ main() {
tmpdir=$(mktemp -d /tmp/scalingo_cli_XXX)
trap "clean_install ${tmpdir}" EXIT

version=$(curl --silent https://cli-dl.scalingo.com/version | tr --delete ' \t\n')
version=$(curl --silent https://cli-dl.scalingo.com/version | tr -d ' \t\n')
Copy link
Member

Choose a reason for hiding this comment

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

question(non blocking): Maybe add a comment everywhere you use short option to explain the reason? As the Scalingo good practice is to use long options, we will maybe be tempted to update these options again in the future.

@curzolapierre curzolapierre force-pushed the fix/1001/fix_install_script_for_mac branch from 84e228d to 86597d7 Compare September 7, 2023 13:18
@curzolapierre curzolapierre merged commit cc57e46 into master Sep 7, 2023
@curzolapierre curzolapierre deleted the fix/1001/fix_install_script_for_mac branch September 7, 2023 13:19
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.

[MacOS] Install script failed due to usage of long parameters
2 participants