Skip to content
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

API change suggestion: rely less on step env and move to step inputs (with) to select operation mode #221

Open
filmaj opened this issue Jun 28, 2023 · 1 comment
Labels
Milestone

Comments

@filmaj
Copy link
Contributor

filmaj commented Jun 28, 2023

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 the chat.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 on env variables passed to the Action step. I propose to expand action.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 new with input entry to specify operation mode), which is why I believe this would be a semver major change.

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 of token, workflow_webhook or incoming_webhook, and
    • depending on which mode specified, additional token and channel-id (if mode: token selected) or webhook (if one of the webhook modes are selected) inputs.
  • 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 a channel-id.
    • If operating in one of the other two webhook-based modes, consumers must specify a webhook input as a URL.

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 of HTTP_PROXY and HTTPS_PROXY environment variables as a de-facto standard across many environments and languages.
@filmaj filmaj added this to the 2.0 milestone Jun 28, 2023
@seratch
Copy link
Member

seratch commented Jun 28, 2023

I like the idea of having the new 'mode' input since v2. It should be easier to understand for everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants