-
Notifications
You must be signed in to change notification settings - Fork 181
feat: add Slack CLI command runner workflow and installer composite action #489
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
base: ewanek1-slack-cli-command-runner-v2
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,130 @@ | ||||||||||||||||||||||||||||||||
| name: Slack CLI Installation and Command Runner | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| name: Slack CLI Installation and Command Runner | |
| name: "Slack: Run a CLI command" |
🌲 suggestion: To match the action.yml file for other techniques!
Outdated
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.
| description: Download and cache the Slack CLI and run the input command | |
| description: "Install the Slack CLI to run a command" |
🌞 suggestion: Perhaps idea on wording - I'm hoping to leave magic behind the scenes with this change but am not sure of the verbiage.
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 like this! It's less verbose and more user-centric imo 🙇♀️
Outdated
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.
| description: "Slack CLI command to run" | |
| description: "Slack CLI command to run. Should not include the 'slack' prefix." |
📚 suggestion: We can document some of these requirements here for the code curious! I'm never sure if these are surfaced elsewhere though...
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.
📚 suggestion: Also I think the descriptions for inputs and outputs can use more worded sentences with end punctuation. Starting with this now makes later updates more clear I find.
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 this feedback - agreed! 🙌
Outdated
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.
| cli_version: | |
| cli-version: |
🐳 suggestion: It's unclear in official documentation what the recommended pattern is for input attributes, but I think "kebab" case is used most often, but it might be nice to change these in this action.yml file?
Outdated
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.
| default: "latest" | |
| default: "3.x.x" |
🚀 suggestion(non-blocking): I'm hoping we can soon land floating version in slackapi/slack-cli#199! For now the installation script should default to the latest without such specifier though.
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.
| app_id: | |
| description: "App ID" | |
| type: string | |
| default: "" | |
| required: false |
🪓 question: For now is this alright to remove? I believe it might conflict with a separate --app flag?
Outdated
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.
| success: | |
| description: "Whether the command ran successfully" | |
| value: ${{ steps.run-slack-cli-command.outputs.success }} | |
| ok: | |
| description: "Whether the command ran successfully" | |
| value: ${{ steps.run-slack-cli-command.outputs.ok }} |
📺 suggestion: To match a similar value of other techniques-
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.
| success: | |
| description: "Whether the command ran successfully" | |
| value: ${{ steps.run-slack-cli-command.outputs.success }} | |
| success: | |
| description: "If the command completed without an error." | |
| value: ${{ steps.run-slack-cli-command.outputs.success }} |
📝 suggestion: Some quick ideas on the description here too.
Outdated
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.
| command_executed: | |
| description: "Command ran" | |
| value: ${{ steps.run-slack-cli-command.outputs.command_executed }} |
🪓 suggestion: Hmm... I'm not sure if we want to return this in addition to what the logs might show?
🗣️ ramble: There's a chance modifications from the verbose option don't appear with this and using the input command I believe will be alright for most use cases.
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.
Hm yea I echo back the command input stored from inputs command. I do have command_executed which is purely for the test file to ensure some of the outputs are as expected. If it's removed some of the tests might not be as rigorous though 🤔
Outdated
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.
| stdout: | |
| description: "Command output" | |
| value: ${{ steps.run-slack-cli-command.outputs.stdout }} | |
| output: | |
| description: "Command output" | |
| value: ${{ steps.run-slack-cli-command.outputs.output }} |
📫 suggestion: We might want to capture both stdout and stderr with this?
Outdated
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.
| uses: actions/cache@v4 | |
| uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 # v4.2.4 |
📍 suggestion: Let's use a pinned version here to avoid unexpected upstream changes!
Outdated
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.
💰 praise: Super nice to be landing this in a first release. While installation is somewhat fast I think restoring from cache is often faster!
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.
🤖 question: I notice the ~ is used when checking this path but the $HOME variable is used following. Would using the $HOME variable here be possible too?
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.
👾 question: Are these steps needed just if the cache was found? Unsure if the installation script has similar effects or if this could be skipped...
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.
Ah yes seems like I would need to move the action to the root level so slack-github-action/cli/action.yml. Instead of having it under .github/resources/.actions
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.
🪬 suggestion: We might prefer to use an explicit token input of this workflow instead of checking environment variables
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.
Let me know if I misunderstand, but wouldn't the user need to repeatedly put their token as an input? Or is it still saved in secrets so they don't need to regularly retrieve it?
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.
@ewanek1 You're right that the token input would be more verbose... But I think this makes workflows more clear when different tokens are needed in separate steps! 👾
I'm curious if we can fallback to environment variables if a token isn't provided in inputs but the order of which token to use, if app and bot tokens are options, might be confusing for other techniques too.
Also, I also confirmed that a token provided by flag isn't saved to file by the CLI to avoid later unexpected happenings with a service token, so this might be similar to current behavior as well?
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.
⌛ question: Is this used as a variable or can we also remove it?
| VERBOSE: ${{ inputs.verbose }} |
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 catching this I never used this and it can be removed 😄
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.
| VERBOSE: ${{ inputs.verbose }} | |
| run: | | |
| cmd="slack ${{ inputs.command }}" | |
| if [ "${{ inputs.verbose }}" == "true" ]; then | |
| cmd="$cmd --verbose" | |
| fi | |
| cmd="$cmd --skip-update" | |
| SLACK_SKIP_UPDATE: "true" | |
| VERBOSE: ${{ inputs.verbose }} | |
| run: | | |
| cmd="slack ${{ inputs.command }}" | |
| if [ "${{ inputs.verbose }}" == "true" ]; then | |
| cmd="$cmd --verbose" | |
| fi | |
🌐 suggestion(non-blocking): Using an environment variable instead might make this logic more clear?
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.
Agreed - thanks!
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.
| echo "Command output: $output" | |
| echo "Exit code: $exit_code" | |
🪓 thought: If these are duplicating outputs it might be alright to remove?
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'll check if the exit code is necessary! I believe Actions resurfaces the error anyways.
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.
🍃 question: Do the outputs from this step also map to the outputs of the workflow? I notice a different id is used here and was not sure how these are mapping at this time...
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 an oversight thank you!!
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,160 @@ | ||||||||
| name: Slack CLI Command Runner Tests | ||||||||
|
|
||||||||
| on: | ||||||||
| pull_request: | ||||||||
|
|
||||||||
| jobs: | ||||||||
| test-all: | ||||||||
| runs-on: ubuntu-latest | ||||||||
| steps: | ||||||||
| - uses: actions/checkout@v4 | ||||||||
|
||||||||
| - uses: actions/checkout@v4 | |
| - name: Checkout the current code | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
👁️🗨️ suggestion: Guards against unexpected changes upstream!
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.
🧪 suggestion: It might be interesting to run this test across multiple runners - ubuntu, mac, and windows?
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.
Yes!
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.
🧪 suggestion(non-blocking): We should follow up with a test that uses both the --app and token value to make sure all things reach the Slack API as expected:
manifest --app $SLACK_APP_ID
🗣️ ramble: I'm not certain if this environment variable can be gathered from the env but that'd be a nice thing to find in test I hope too?
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.
Agreed similar to what I have for verbose.
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.
⭐ praise: We've discussed this setup and I think the separated action definition is a nice pattern to use for the CLI technique! Thanks for encouraging this!
👁️🗨️ thought: The exact calling syntax escapes me but for releases it'd be so interesting to find this pattern in calling workflows: