Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 797 forks source link

action-test-results-to-slack doesn't support workflow_dispatch #39412

Open testlabauto opened 4 days ago

testlabauto commented 4 days ago

Impacted plugin

None / Other

Quick summary

I just started using this plugin and noticed that all workflow_dispatch events will be grouped because the eventName is not handled.

I think it would be possibly sufficient to add:

if ( eventName === 'workflow_dispatch' ) {
    msgId = `workflow_dispatch-${ Date.now() }`;
}

at: https://github.com/Automattic/action-test-results-to-slack/blob/trunk/src/message.js#L98

But, of course, another implementation would be fine! Just trying to avoid all workflow_dispatch being grouped as it is easy to lose track of those messages in the history.###

Steps to reproduce

  1. Run a workflow_dispatch workflow using the plugin
  2. Make code changes
  3. Run it a second time

A clear and concise description of what you expected to happen.

Separate messages are sent to slack.

What actually happened

All workflow_dispatch messages are grouped.

Impact

One

Available workarounds?

No and the platform is unusable

If the above answer is "Yes...", outline the workaround.

No response

Platform (Simple and/or Atomic)

No response

Logs or notes

No response

anomiex commented 4 days ago

I'd recommend someone taking on this request to allow for using appropriately named inputs to optionally specify a sha and source repo, in the same manner that the handling for 'repository_dispatch' uses client_payload.

testlabauto commented 1 day ago

Thank you! in the meantime, if I use a fork, is this what you are suggesting @anomiex ?

    if ( eventName === 'workflow_dispatch' ) {
        target = `for event _*${ payload.action }*_`;

        if ( payload.client_payload?.sha ) {
            const upstreamSha = payload.client_payload.sha;
            msgId = `workflow_dispatch-${ upstreamSha }`;
            contextElements.push(
                getTextContextElement( `Last commit: ${ upstreamSha.substring( 0, 8 ) }` )
            );

            if ( payload.client_payload?.repository ) {
                const commitUrl = `${ serverUrl }/${ payload.client_payload.repository }/commit/${ upstreamSha }`;
                buttons.push( getButton( `Commit ${ upstreamSha.substring( 0, 8 ) }`, commitUrl ) );
            }
        } else {
            msgId = `workflow_dispatch-${ Date.now() }`;
        }
    }
anomiex commented 1 day ago

If you're going to make a fork, you could also submit a PR for us to review. 🙂

As for the code, it looks ok to me except that you'd want to do payload.inputs? rather than payload.client_payload? everywhere in there. That should correspond to a workflow yaml something like

on:
  workflow_dispatch:
    inputs:
      sha:
        description: 'Commit tested'
        type: string
      repository:
        description: 'Repository tested'
        type: string
testlabauto commented 1 day ago

Thank you! Will do! I will test on my fork first and then submit a PR.