-
Notifications
You must be signed in to change notification settings - Fork 24
docs: update Slack CLI and Install Scripts to use the new https://slack.dev docs #34
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
+ Coverage 62.90% 62.92% +0.01%
==========================================
Files 210 210
Lines 22149 22149
==========================================
+ Hits 13933 13937 +4
+ Misses 7131 7126 -5
- Partials 1085 1086 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Added a few comments to help the kind reviewers! 📚
|
|
||
| [circleci]: ../.circleci/config.yml | ||
| [commands]: https://api.slack.com/automation/cli/commands | ||
| [commands]: https://tools.slack.dev/slack-cli/reference/commands/slack |
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.
note: We might be able shorten this URL in the future to:
https://tools.slack.dev/slack-cli/reference/commands
| $ {{.CommandPath}}{{if eq .Name (GetProcessName)}} <command>{{end}} <subcommand> --help | ||
| For guides and documentation, head over to {{LinkText "https://api.slack.com/automation"}}{{end}} | ||
| For guides and documentation, head over to {{LinkText "https://tools.slack.dev/slack-cli"}}{{end}} |
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.
thought: I think a more appropriate page is the new, Slack CLI homepage! ✨
| if len(triggerFilePaths) <= 0 { | ||
| clients.IO.PrintInfo(ctx, false, style.SectionSecondaryf( | ||
| "No trigger definition files found\nLearn more about triggers: https://api.slack.com/automation/triggers/link", | ||
| "No trigger definition files found\nLearn more about triggers:\nhttps://tools.slack.dev/deno-slack-sdk/guides/creating-link-triggers", |
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.
note: I added a newline \n since the URL is a little longer.
| var checkForUpdatesFunc = checkForUpdates | ||
|
|
||
| const changelogURL = "https://api.slack.com/automation/changelog" | ||
| const changelogURL = "https://docs.slack.dev/changelog" |
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.
note: MVP change!
| Remediation: fmt.Sprintf(`Learn about building apps with the Deno Slack SDK: | ||
| https://tools.slack.dev/deno-slack-sdk | ||
| If you are using a Bolt framework, add a deploy hook then run: %s | ||
| Otherwise start your app for local development with: %s`, | ||
| style.Commandf("deploy --experiment=bolt", true), | ||
| style.Commandf("deploy", true), |
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.
note: Updated an old Bolt Experiment reference, how does it read?
| echo "🛑 Error: This installer is only supported on Linux and macOS" | ||
| echo "🔖 Try using a different installation method:" | ||
| echo "🔗 https://api.slack.com/automation/cli/install" | ||
| echo "🔗 https://tools.slack.dev/slack-cli" |
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.
note: Install script updates! So we'll want to deploy those next release 🚀
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 is a good landing page to point to IMO since the exact error at this point might not be known to the installer.
Once more I'm optimistic that issues will be useful in debugging issues with the installation script when these are encountered 🙏 ✨
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.
Edit: I also look forward to the next release! 🚀
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.
Yea, I'm looking forward to weaving GitHub Issues into more of the output from the CLI!
| if [ -z "$LATEST_SLACK_CLI_VERSION" ]; then | ||
| echo "🛑 Error: Installer cannot find the latest Slack CLI version!" | ||
| echo "🔖 Check the status of https://api.slack.com and try again" | ||
| echo "🔖 Check the status of https://slack-status.com/ and try again" |
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.
note: Cool with this? The URL seems more appropriate?
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 is a nice change! IIRC the status of CLI downloads will match other services of Slack and might not be related to the api.slack.com page.
Tracking status problems down from reported issues seems ideal to me now that this can be reported via GitHub issues.
Before this codebase was open source we might've been reliant on sharing updates through other channels but I do not recall if we've had to do that...
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.
Edit: I realize this particular change is after erroring while gathering metadata from the "api.slack.com" pages!
This makes even more sense to use the https://slack-status.com/ site instead in case the linked subdomain might error somehow else 😅
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.
Good points! If we find that https://slack-status.com/ doesn't reflect our file serving endpoints (I imagine it does) then we can always update this error to ask people to submit a GitHub Issue. This would allow us to investigate it further.
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.
@mwbrooks All of these updates are so much appreciated 📚 ✨
These pages and related have been in constant motion the past few days but now seems like the right time to make these updates.
I did leave a few comments of links to "api.slack.com" that might have alternative "docs.slack.dev" endpoints we might use instead?
That's no blocker but I do think we should make these replacements where possible. I'm adding a commit that updates a few more links in comments and related too, but please revert that if it's not making the right changes 🤖
|
|
||
| ## 📄 Hook Specification | ||
|
|
||
| Hooks are entry points for the CLI to initiate inter-process communication with the SDK. SDKs should implement one hook: `get-hooks`. It is the recommended approach for SDKs to enable communication with the CLI (for more details, see the [Hook Resolution](#hook-resolution) section). |
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.
👾
| ### 🪝 `build` (optional) | ||
|
|
||
| Implementing this hook allows for the CLI to [deploy function code to Slack's managed infrastructure](https://api.slack.com/automation/deploy). The work of assembling the application bundle according to Slack's application bundle format is delegated to the SDK via this hook. | ||
| Implementing this hook allows for the CLI to [deploy function code to Slack's managed infrastructure](https://tools.slack.dev/deno-slack-sdk/guides/deploying-to-slack). The work of assembling the application bundle according to Slack's application bundle format is delegated to the SDK via this hook. |
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.
📣 I'm glad the specification is included with the repo.
Some sidethought makes me think we might want to include the build hook before deploy for bolt apps too.
I don't mean to sidetrack this PR though! We could instead discuss this on a separate PR around this document 🤖
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.
Definitely want consistency in how the hooks are executed - it shouldn't skip hooks based on the framework, instead the framework can have a no-op (exit 0) script.
| if [ -z "$LATEST_SLACK_CLI_VERSION" ]; then | ||
| echo "🛑 Error: Installer cannot find the latest Slack CLI version!" | ||
| echo "🔖 Check the status of https://api.slack.com and try again" | ||
| echo "🔖 Check the status of https://slack-status.com/ and try again" |
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 is a nice change! IIRC the status of CLI downloads will match other services of Slack and might not be related to the api.slack.com page.
Tracking status problems down from reported issues seems ideal to me now that this can be reported via GitHub issues.
Before this codebase was open source we might've been reliant on sharing updates through other channels but I do not recall if we've had to do that...
| echo "🛑 Error: This installer is only supported on Linux and macOS" | ||
| echo "🔖 Try using a different installation method:" | ||
| echo "🔗 https://api.slack.com/automation/cli/install" | ||
| echo "🔗 https://tools.slack.dev/slack-cli" |
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 is a good landing page to point to IMO since the exact error at this point might not be known to the installer.
Once more I'm optimistic that issues will be useful in debugging issues with the installation script when these are encountered 🙏 ✨
| echo "🛑 Error: This installer is only supported on Linux and macOS" | ||
| echo "🔖 Try using a different installation method:" | ||
| echo "🔗 https://api.slack.com/automation/cli/install" | ||
| echo "🔗 https://tools.slack.dev/slack-cli" |
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.
Edit: I also look forward to the next release! 🚀
Co-authored-by: Eden Zimbelman <[email protected]>
Co-authored-by: Eden Zimbelman <[email protected]>
Co-authored-by: Eden Zimbelman <[email protected]>
Co-authored-by: Eden Zimbelman <[email protected]>
Co-authored-by: Eden Zimbelman <[email protected]>
|
@zimeg As always, thanks for the thorough review! I've added all of your suggestions and it inspired me to do another search through our api.slack.com references. Commit c689906 also updates our app manifest reference to this new page: |
Summary
🤾🏻 Kudos to @zimeg for pointing this out!
This pull request updates all references to https://api.slack.com to https://slack.dev, when available.
In most cases, this just prevents a redirect from happening.
The main benefit is the promote our new CHANGELOG page whenever the Slack CLI has an update:
Requirements