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

CI: Display LightHouse stats in nicer way #692

Closed
wants to merge 1 commit into from
Closed

CI: Display LightHouse stats in nicer way #692

wants to merge 1 commit into from

Conversation

HarshCasper
Copy link

Fixes #582

This PR:

  • Formats a comment displaying the LightHouse stats in a visual and tabular manner
  • Uses marocchino/sticky-pull-request-comment@v2.2.0 to put up the comment on the PR
  • Adds a check to run the LightHouse Statistic Comment only over a GitHub PR

@HarshCasper
Copy link
Author

HarshCasper commented Mar 19, 2022

Here are the LightHouse Stats:

Category Score
🟢 Performance 96
🟢 Accessibility 97
🟢 Best practices 100
🟢 SEO 98
🔴 PWA 40

Currently, the comment is not being put up, since the GitHub Token is not accessible by integration (since the token is not available on forked repositories). Here are the logs: https://github.com/jupyter/jupyter.github.io/runs/5612667281?check_suite_focus=true

- name: LightHouse Statistic Comment
if: github.event_name == 'pull_request'
id: lighthouse_statistic_comment
uses: marocchino/sticky-pull-request-comment@v2.2.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please use gh pr comment as it does not require installation of a third-party action?

@krassowski
Copy link
Member

Thank you, this looks like a good approach! As mentioned in code review, I would suggest to use gh which is pre-installed on GitHub runners to reduce third-pary dependencies.

Also, the following permissions may be needed:

permissions:
  pull-requests: write

@ivanov
Copy link
Member

ivanov commented Dec 6, 2023

@HarshCasper and @krassowski, just doing some 🧹 cleaning: do you think this is still worth pursuing, or should we close it?

@krassowski
Copy link
Member

My guess is that with actions being quite dynamic ecosystem we should reconsider the specific actions proposed. This PR is valuable but if it staying in unfinished state discourages other contributors let's close it. Also for now the priority would be fixing ci in the first place

@ivanov
Copy link
Member

ivanov commented Dec 7, 2023

Also for now the priority would be fixing ci in the first place

That makes sense, done in #749.

I don't mind it being open if this can be used as a starting point by someone. I'll just convert it to draft since there are some unaddressed third-party-actions here.

@ivanov ivanov marked this pull request as draft December 7, 2023 00:42
@HarshCasper HarshCasper closed this by deleting the head repository Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Lighthouse CI App integration for nicer checks display?
3 participants