-
Notifications
You must be signed in to change notification settings - Fork 24
feat: output a link to the issue tracker if installation scripts fail #148
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
=======================================
Coverage 63.38% 63.38%
=======================================
Files 212 212
Lines 22333 22333
=======================================
Hits 14156 14156
+ Misses 7093 7092 -1
- Partials 1084 1085 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🗣️ Powershell changes are being made in cbeed29 might resemble: Edit: Punctuation in the output error was removed to be more consistent > .\scripts\install-windows.ps1 # Installs the latest version
> .\scripts\install-windows-dev.ps1 -Version 3.0.0 # Installs a set version
> .\scripts\install-windows-dev.ps1 # Expected error without a download |
zimeg
left a comment
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.
🔏 A few notes on the changes for the wonderful reviewers! ✨
scripts/install-dev.sh
Outdated
|
|
||
| feedback_message() { | ||
| if [ $(command -v $SLACK_CLI_NAME) ]; then | ||
| if [ $? -eq 0 ] && [ $(command -v $SLACK_CLI_NAME) ]; then |
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.
📣 This function is now called on errors too - we check the status before it's called to change outputs here!
| else | ||
| echo "Slack CLI requires a valid semver version number." >&2 | ||
| exit 1 | ||
| return 1 |
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.
📝 The exit command is replaced with a return to avoid ending the script altogether for later logic.
| Write-Host " Then, authorize your CLI in your workspace with ``$confirmed_alias login``.`n" | ||
| } | ||
| catch { | ||
| throw "`nSlack CLI is not installed.`nPlease reach out to [email protected] to share the issues you are facing.`nMeanwhile you can try the manual installation: https://tools.slack.dev/slack-cli.`n" |
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.
💌 We now have an issue tracker to direct the most kind folks to!
mwbrooks
left a comment
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.
✅ Thanks for the quick turnaround on this @zimeg 👏🏻 I've left a minor suggestion but non-blocking!
Co-authored-by: Michael Brooks <[email protected]>
|
📣 Tested this with a remote file on Windows using: |
|
@mwbrooks I appreciate so much the review 🙏 One more change was made to catch the cases of an unknown version being passed to the script in 0d1f812 which catches subshell errors: |
|
@mwbrooks With tests passing now, let's merge this for the next release 🚢 💨 I remain so interested in following up with a few more improvements, but if fast fixes are needed in the meantime let's address those first! |

Summary
This PR outputs a link to the issue tracker if installation scripts fail. Following reported errors of #147.
Preview
Reviewers
With these changes change the environment for testing:
Notes
Before requesting a review I plan to make similar changes on Windows.
Requirements