-
Notifications
You must be signed in to change notification settings - Fork 174
Description
Open discussion for a suggestion on a version 2 of this Action, which would include a breaking API change from this action's API today.
Context / Introduction
The issues have been piling up in this repo and we at Slack DevRel have done the bare minimum - if even that - to maintain this project. It is a popular project with lots of use, but it is a frustrating experience to use this Action, as can be seen by the discussion in the issues.
I recently tried to tackle some of the issues in this repo but I found myself consistently hitting problems with basic project maintenance tasks such as running the tests (see #219) and understanding how the code works. The latter is more a function of how our DevRel engineering team operates: we have many open source repositories to maintain and stay on top of, so our team is constantly context switching between tasks and projects. This means quickly and easily "remembering" how different projects work and what their design is is important for the long-term maintenance of projects.
Problem
There are many problems with this Action today (just flip over to the Issues tab to see!), but one problem above all underlines them: I believe v1 of this Action's API uses environment variables as a sub-optimal way of defining Action inputs. As a result, the code for this Action relies on inspecting process environment variables to determine whether it should use the chat.postMessage
Slack HTTP API or POST to a webhook URL. That is: environment variables determine this Action's behaviour. This leads to difficulty in running the unit tests and integration tests (the jobs specified in this project's .github/workflows/main.yml
) because necessarily we need to exercise all of this Action's features as part of the test suite. As such, the tests need to juggle and set/reset different environment variables to control behaviour to test all the code paths.
Proposal
TL;DR: define Action inputs to determine behaviour and only read data from environment variables.
- This Action to use environment variables (
env
within a GitHub Action Workflow) to read sensitive data from. Nothing more. Logic is not conditional on the existence of environment variables. - This Action to use inputs (consumers would specify via
with
within a GitHub Action Workflow for a specific job step) to determine behaviour, that is, which of the three 'modes' this Action can operate in: using a bot token and POSTing to thechat.postMessage
API, POSTing to a webhook trigger as part of Slack's Workflow Builder, and finally POSTing to a Slack application incoming webhook.- This Action specifies many of these inputs already in its
action.yml
. However, switching between the three modes is purely dependent onenv
variables passed to the Action step. I propose to expandaction.yml
with inputs defining which mode to operate in. Technically from an API perspective, this could be considered a semver-minor change, but I believe this change would break backwards compatibility (consumers would have to add a newwith
input entry to specify operation mode), which is why I believe this would be a semver major change.
- This Action specifies many of these inputs already in its
As for what the inputs should be, some initial ideas:
- More explicit. Consumers specify at least two, but up to three, mandatory inputs:
- a required
mode
input to have a value of one oftoken
,workflow_webhook
orincoming_webhook
, and - depending on which
mode
specified, additionaltoken
andchannel-id
(ifmode: token
selected) orwebhook
(if one of the webhook modes are selected) inputs.
- a required
- More implicit. Consumers specify at least one, but up to two, mandatory inputs:
- If operating in bot-token mode, consumers must specify at least a
token
input housing the bot token to use, and achannel-id
. - If operating in one of the other two webhook-based modes, consumers must specify a
webhook
input as a URL.
- If operating in bot-token mode, consumers must specify at least a
The other optional inputs like slack-message
, payload
and payload_file_path
could remain as-is.
New Automation Platform
Slack's new Automation Platform is out and I think this proposal should also include a refresh to focus less on the 'legacy' Workflow Builder in Slack that used Steps from Apps, and instead focus more on the new features the latest Workflow Builder provides. In particular, updating the instructions and setup to be compatible with the updated Workflow Builder's webhook triggers. There should be full compatibility between this Action and the old Workflow Builder webhook triggers compared with the refreshed Workflow Builder's webhook triggers, as far as I can tell.
Anything Else?
- Are there are other inputs and outputs we should consider adding or removing as part of a v2 release?
- Should
HTTPS_PROXY
remain as an environment variable, or should we move to an input? This one I am not sure about as use ofHTTP_PROXY
andHTTPS_PROXY
environment variables as a de-facto standard across many environments and languages.
- Should