flowwer-dev / pull-request-stats

Github action to print relevant stats about Pull Request reviewers
MIT License
366 stars 79 forks source link

Documentation: How to make premium features work after sponsoring #38

Closed pattwell closed 2 years ago

pattwell commented 2 years ago

I may be missing something, but even after sponsoring, the Slack Integration still returns the output Slack integration is a premium feature, available to sponsors.

It is not clear to me how to configure things so that the action is aware that our Organisation is already a sponsor and allow the Slack action to run. Some documentation to address this would be awesome!

Thanks.

manuelmhtr commented 2 years ago

Hi @pattwell, Sorry for the late reply. I think the problem is the sponsorship is configured as private, then the action is asking the API if the organization is a sponsor but getting a negative response.

The quickest fix is to change the sponsorship from private to public, however if you prefer to keep it private just tell me and I'll think of a workaround to make it work.

PD. Thank you very much to the company and the team for being sponsors. As an open source developer is really encouraging and helps me keep pushing this project forward πŸ˜„

Looking forward to your comments,

pattwell commented 2 years ago

Hi @manuelmhtr ,

Thank you! That makes total sense and should have been obvious in retrospect. We changed our Sponsorship to Public and the action ran successfully now :)

I am still having trouble getting the Slack message to post, however.

Perhaps another thing to request in the README is a bit more detail on what needs to be set up in the Webhook itself in Slack - as it requires you to add at least one "step" but I'm not sure whether I should be configuring a "Post a message" step to take a variable from the Webhook content. I tried catching a var as a few things (message, data, blocks) but it fails. My js knowledge isn't good enough to reverse engineer your code I think πŸ˜„

My action looks like this:

name: "Pull Request Stats"

on:
  workflow_dispatch:

jobs:
  stats:
    runs-on: ubuntu-latest
    steps:
      - name: Run pull request stats
        uses: flowwer-dev/pull-request-stats@master
        with:
          organization: Accredible
          token: ${{ secrets.ORG_READ_PAT }}
          slack-webhook: ${{ secrets.PR_STATS_SLACK_WEBHOOK_URL }}
          slack-channel: '#testing'

And my webhook:

Screenshot 2022-06-29 at 10 09 50

Hope this makes sense, please let me know if I'm missing a bit of config!

manuelmhtr commented 2 years ago

Hi @pattwell I'm glad it worked :D

About the slack configuration, I think you are creating a Workflow which is a bit different.

What you need to create is an "App" and then create an "Incoming Webhook" for that app. At the end you should get an URL. That URL is the one that you need to assign to your PR_STATS_SLACK_WEBHOOK_URL secret.

I found this tutorial, which I think explains pretty well how to do it.

Please tell if that works for you.

PD. Sorry for not having a clearer documentation on the Slack Integration. You are the 2nd company using it, and you are helping me a lot to improve it.

pattwell commented 2 years ago

@manuelmhtr Ah thank you, silly me! I read "Create a webhook" and my mind instantly went to the Workflow Webhooks as I have used them a lot in the past. Since this is the interface to select a Webhook type workflow, it didn't cross my mind that I was looking at totally the wrong thing:

Screenshot 2022-06-30 at 09 26 00

That's my fault for making assumptions and not reading the help article you linked carefully enough.

I was able to get the Slack messages working now πŸ₯³ I did get some 400 errors at first but managed to figure out that without the limit set to 15 or less (not sure the exact number, I just did some trial and error) the webhook message was probably too long and the Slack API rejected the request.

No need to apologise, I am also sorry for misunderstanding! But I suppose users getting things wrong is certainly the best way to know what to change in Documentation πŸ˜†


In summary, my gentle suggestions for improvement would be:

Thank you so much for your swift help and responses with this, we really appreciate it!

pattwell commented 2 years ago

Hello again! Sorry for lots of comments, but just sharing some further findings after more testing.

After the inital success, I am now scheduling the action to run once for our Backend developers' repos, and separately for our Frontend developers' repos, as this makes the stats easier to analyse. For these I am specifying 3 repos each, eg:

      - name: Backend Stats
        uses: flowwer-dev/pull-request-stats@master
        with:
          repositories: <repo1>,<repo2>,<repo3>
          limit: 10
          token: ${{ secrets.REVIEW_BOT_AUTOSYNC_PAT }}
          slack-webhook: ${{ secrets.PR_STATS_SLACK_WEBHOOK_URL }}
          slack-channel: '#testing'

I noticed that the Slack message says "Stats of the last 30 days forΒ repo1 and repo2", so repo3 is missed from the message. All 3 repos are included in the actual statistics found, which is the important part, but the Slack message is a bit misleading.

Additionally, I had to make the limit = 10 as I got 400 errors once again when trying with limit = 15 and with limit = 12.

Screenshot 2022-06-30 at 12 09 47

This is a bit confusing since I assumed the issue was message size, an when analysing all repos in the Organisation it works with limit 15, perhaps I am wrong? πŸ€” It's slightly hard to tell as the debug logging doesn't show the full API error I think.

Sorry if this isn't the right place to raise this, I can start a separate issue if that helps, but since the previous context of this thread was relevant I thought I would add it here. Please don't feel rushed to respond quickly, this is not a high priority issue.

Thank you again, this action is super interesting and it's fun to test out!

manuelmhtr commented 2 years ago

Hi @pattwell! Happy to hear it's working now πŸ™Œ and thank you very much for all the debugging and feedback (don't worry, this issue is fine to talk about all related topics).

About each comment:

Perhaps note also that the Webhook URLs should be treated as sensitive values

You are right, let me check how can I specify to Github that threat this value as sensitive. This is to prevent it from being printed during debugging, right? or am I missing something else?


I noticed that the Slack message says "Stats of the last 30 days for repo1 and repo2", so repo3 is missed from the message.

Ooops! I added this section of the message recently and I think there's a bug, specifically when there are 3 repos (because 3 is the trigger to switch the message for "repo1, repo2 and X more repos" when there are many).


Additionally, I had to make the limit = 10 as I got 400 errors once again when trying with limit = 15 and with limit = 12.

Omg, is super annoying to guess which number should be assigned as limit U_U

I think what takes the most space and makes it a variable number are the URLs (which may take up to 1,024 characters). Let me think of a workaround. It may be trimming the last rows automatically to fit Slack's limit, or maybe by somehow shortening the urls (this would allow more rows to be printed) πŸ€”.


Let me work on those improvements (hopefully this weekend) πŸ˜‰

Any other feedback is very welcome. Thanks!

pattwell commented 2 years ago

Hey again @manuelmhtr :)

You are right, let me check how can I specify to Github that threat this value as sensitive. This is to prevent it from being printed during debugging, right? or am I missing something else?

For this I just mean that in the README you show an example of how people could set up the slack integration:

      - name: Run pull request stats
        uses: flowwer-dev/pull-request-stats@master
        with:
          slack-webhook: 'https://hooks.slack.com/services/T00000000/B00000000/XXXXXXXXXXXXXXXXXXXXXXXX'
          slack-channel: '#mystatschannel'

But it might be better to show this as the example:

      - name: Run pull request stats
        uses: flowwer-dev/pull-request-stats@master
        with:
          slack-webhook: ${{ secrets.webhook }}
          slack-channel: '#mystatschannel'

This will encourage people to use the Webhook URL via Repo secrets rather than committing it to their source code. It will work either way but it's best practice to use secrets and nice and easy to do so.


Ooops! I added this section of the message recently and I think there's a bug, specifically when there are 3 repos (because 3 is the trigger to switch the message for "repo1, repo2 and X more repos" when there are many).

This is such a relatable programming issue lol, totally understandable and hopefully an easy fix πŸ˜„


Other than those points it's working well now, I will let you know if we have any other feedback but I think it's all good.

Thanks again for your quick responses! I look forward to seeing some progress when you're able to find some time.

manuelmhtr commented 2 years ago

Hi @pattwell I took more time than expected but finally came with a solution for the characters limit on Slack, it's available on v2.4.5.

The workaround was to split the message into chunks and post multiple times while preserving the order. This way it still looks like 1 single post and you can remove the limit option.

I also improved the read based on your suggestions :)

I'll close this issue but please feel free to reopen it if you have any question.

Best,

pattwell commented 2 years ago

Hi @manuelmhtr,

Sorry for such a late response! Thank you so much for the updates, the limit does seem to work much better now πŸ˜„

The only remaining thing, I think, is the way the repos are listed if we choose exactly 3 repositories to scan. That is, it still shows as "Stats of the last 30 days for repo1 and repo2", with repo3 missed from the message.

Thanks :)