mattermost / mattermost-plugin-starter-template

Build scripts and templates for writing Mattermost plugins.
https://developers.mattermost.com/extend/plugins/
Apache License 2.0
128 stars 120 forks source link

Fix ununsed param #189

Closed mickmister closed 10 months ago

mickmister commented 10 months ago

Summary

The scheduled CI job that runs every day has been failing for about a month due to this linting rule. @hanzei Any way we can be notified of this, maybe through a webhook to Mattermost?

hanzei commented 10 months ago

A webhook makes sense. Actually, I would like to see such a notification for all plugin repos.

mickmister commented 10 months ago

@hanzei Seems that we would get that for all plugin repos automatically by implementing this in the centralized plugin CI workflow. From this SO answer https://stackoverflow.com/a/63314229, it seems we can do something like this:

if: always() && (steps.ci/lint.outcome == 'failure')

We would want it to run if any of the steps fail though. Here's another suggested solution https://stackoverflow.com/a/74562058

if: ${{ always() && contains(needs.*.result, 'failure') }}

Given that we will trigger some step due to failure of a previous step, we would need a way to notify Mattermost. We could have a separate action that sends a message to Mattermost via webhook, accepting parameters like WEBHOOK_URL and MESSAGE. @toninis What do you think about this?

toninis commented 10 months ago

@hanzei Seems that we would get that for all plugin repos automatically by implementing this in the centralized plugin CI workflow. From this SO answer https://stackoverflow.com/a/63314229, it seems we can do something like this:

if: always() && (steps.ci/lint.outcome == 'failure')

We would want it to run if any of the steps fail though. Here's another suggested solution https://stackoverflow.com/a/74562058

if: ${{ always() && contains(needs.*.result, 'failure') }}

Given that we will trigger some step due to failure of a previous step, we would need a way to notify Mattermost. We could have a separate action that sends a message to Mattermost via webhook, accepting parameters like WEBHOOK_URL and MESSAGE. @toninis What do you think about this?

The million dollar question is if we need the scheduled nightly builds in the first place . We can enable centralised monitoring for our plugins here

I would suggest a new job with the below config. We avoid always() check here

if: failure() || cancelled()

Check on our delivery repository here for an example

mickmister commented 10 months ago

The million dollar question is if we need the scheduled nightly builds in the first place . We can enable centralised monitoring for our plugins here

Check on our delivery repository here for an example

@toninis Thanks all makes sense to me. I'll take a look at implementing this.


The million dollar question is if we need the scheduled nightly builds in the first place

These jobs are configured per plugin repo, so if we want to stop them it should probably a centralized change.

https://github.com/mattermost/mattermost-plugin-starter-template/blob/5237d354cf30d90ba2938ea9b337b7babee44596/.github/workflows/ci.yml#L3

There have been issues from these scheduled jobs running on forks, because of the resources they're using (mostly because of stored artifacts). There was a fix to turn off the scheduled job in this repo as a first step https://github.com/mattermost/mattermost-plugin-starter-template/pull/180, though was closed in favor of a centralized fix https://github.com/mattermost/actions/pull/10 for the piece related to space used storing artifacts.

Seeing that the scheduled job on this repo wasn't set up to have any indication of it failing, I'm thinking nobody has had any benefit from these jobs running on their forks either. I propose we centralize disallowing running in forks, possibly overridable through a parameter in case someone maintaining a plugin does want to. And if we want to continue running scheduled jobs for our org's repos, maybe we should change to one week instead.