-
Notifications
You must be signed in to change notification settings - Fork 178
feat: add Slack CLI command runner workflow and installer composite action #486
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: main
Are you sure you want to change the base?
Conversation
|
Thanks for the contribution! Unfortunately we can't verify the commit author(s): Eva Wanek <e***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, sign the Salesforce Inc. Contributor License Agreement and this Pull Request will be revalidated. |
| name: "Run Slack CLI command" | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| SLACK_CLI_VERSION: "3.5.2" | ||
| timeout-minutes: 10 | ||
|
|
||
| defaults: | ||
| run: | ||
| shell: bash | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Slack CLI | ||
| uses: ./.github/resources/.actions/slack-cli-installer | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 18 | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install shell-quote | ||
|
|
||
| - name: Execute command via script | ||
| run: | | ||
| echo "Running command: ${{ github.event.inputs.command }}" | ||
| node src/parse-command.js ${{ github.event.inputs.command }} | ||
| env: | ||
| SLACK_SERVICE_TOKEN: ${{ secrets.SLACK_SERVICE_TOKEN }} | ||
| SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} No newline at end of file |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, add a permissions block to the workflow or the specific job to explicitly set the minimal required permissions for the GITHUB_TOKEN. In this case, since the job only checks out code and runs scripts, it only needs contents: read permission. The best practice is to add the following block at the job level (under deploy:), but it can also be added at the root of the workflow if all jobs require the same permissions. No changes to functionality are required, and no additional imports or definitions are needed.
-
Copy modified lines R13-R14
| @@ -12,2 +12,4 @@ | ||
| name: "Run Slack CLI command" | ||
| permissions: | ||
| contents: read | ||
| runs-on: ubuntu-latest |
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.
📝 Leaving a few notes on the current changeset for now, but the steps of this composite action are so promising!
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 want to move these steps to action.yml as part of the compositisation of this action!
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: It might be best to include this technique as part of existing tests workflow?
I'm hoping with a composite action setup we could perhaps test the CLI version command returns a recent version to start. Perhaps too we start running these tests across various os setups to improve coverage for all 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.
Ah yes definitely more testing on the next iteration 😊
WilliamBergamin
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.
This is an awesome first iteration 💯 🚀
We might want to create a new long lived branch to host this new feature instead of merging into main. This should allow us to open PRs against main and cut releases until this feature it ready 🥇
We spoke about triggering continuous integration tests that cover the various combinations of operating systems 🤔 would it be possible to include these in the PR, and have them triggered by the PR creation
Briefly spoke about potentially removing the shell-quote dependency. If we want to maintain the intention behind its usage we could add action inputs to help developers set common flags and provide default values that configure the CLI for CI/CD pipeline usage
- An
appandteaminput could be used to set the--appand--teamflags - By default we could set the
--skip-updateflag totrueand provide askip-updateinput that allows developers to change the default - The
--verboseflag could also default totrueand averboseinput could be used to change its behavior
We could also parse the command to ensure we only add these flag values if they are not already set by the "raw" command
| key: slack-cli-${{ runner.os }}-${{ runner.arch }}-${{ env.SLACK_CLI_VERSION }} | ||
|
|
||
| - name: Add Slack CLI to PATH (Linux/macOS) | ||
| if: runner.os != '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.
Why do we opt to use runner.os != 'Windows' instead of runner.os == 'Linux' || runner.os == 'macOS' used in Install Slack CLI (Linux/macOS)?
Could we potentially merge these steps?
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! I can use a testing matrix and those tests run when the workflow runs. And then set those tests to run on a pull request event.
- I agree, I would change to a more robust parsing library like yargs and put some defaults in. There's some useful CLI flags I think make sense to automatically set to true. Thanks!
- Hm I used that since it was more concise. Although I believe it's better to explicitly mention which OS we support. A user could variant for their runner which may not support bash and this conditional would error. I don't think I can merge these though.
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.slack/bin | ||
| key: slack-cli-${{ runner.os }}-${{ runner.arch }}-${{ env.SLACK_CLI_VERSION }} |
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.
Nice 💯
| node-version: 18 | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install shell-quote |
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.
Based on our discussion and the way we are using this dependency, it may end up being removed, but if we do keep it then we should pin it to a specific version and find a way to easily update it. Dependabot might be useful for this
| node src/parse-command.js ${{ github.event.inputs.command }} | ||
| env: | ||
| SLACK_SERVICE_TOKEN: ${{ secrets.SLACK_SERVICE_TOKEN }} | ||
| SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} No newline at end of file |
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.
In the case of slack run will the CLI override the environment variable set by the developer?
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.
Based on the docs, tokens stored in GitHub secrets or set in the workflow file takes precedence over variables stored in configuration files.
| if (error) { | ||
| throw Error(`Slack CLI Error: ${error.message}`); | ||
| } | ||
| console.log(stdout); |
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.
Should we log stdout before we through the error?
What happens if the error does not have a message field?
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 it seems throwing will exit the current function and the logging might not get reached. I'll make sure to log then throw!
- 'By default, the message property is an empty string,' so I think we're good.
| throw Error("No command provided"); | ||
| } | ||
|
|
||
| console.log('Executing the command:', tokens.join(' ')); |
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.
In a follow up PR it could be nice to have this script follow the same logging level approach as it does with the --verbose flag
|
@WilliamBergamin Dang these are good suggestions. I want to share a few more details that might also be helpful!
I'm somewhat curious also if This makes me want to improve errors more downstream and use environment variables in a few more spots 👾 |
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.
@ewanek1 Sharing more agreement here that this is a great start for iterations to follow! 🚢 💨
I marked this PR as a "draft" so we don't merge it too soon, but please feel free to make changes here or in other PRs and request reviews whenever makes sense. We might also find prereleases as a nice next step 👁️🗨️
Summary
This PR adds a new GitHub composite action and workflow that allows running Slack CLI commands directly.
Requirements