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

GRS Action 🎉🎉 #2537

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

GRS Action 🎉🎉 #2537

wants to merge 2 commits into from

Conversation

Zo-Bro-23
Copy link
Collaborator

The GRS GitHub Action is finally ready (I think)! This PR only updates the documentation, since the action is hosted here. This is necessary because GitHub requires (it's possible to get over this, but they don't recommend it) marketplace actions to have their own repo (see this).

Currently published in the GitHub Marketplace. Use Zo-Bro-23/grs-action@v0 for testing.

Some questions I have:

  • Will there be problems when I migrate the action repo from my personal account to the GRS org? @rickstaa doesn't think so, so I'll perform some tests with him and update this comment accordingly.
  • Should I remove the fetch_multipage option and just enable it by default? @rickstaa thinks so.
  • The action currently uses card types stats, repo, top-langs, and wakatime. repo conflicts with the pin used for the API and stats conflicts with / used for the API, although an empty card type defaults to stats, which makes it compatible with the API. Is it worth renaming repo to pin to maintain backwards compatibility? repo seems more intuitive.
  • Should I copy the docs to the marketplace entry or should I just link to this repo?
  • Should the GitHub token be required (as is currently)? Technically wakatime can be used with a GH Token, but since it's easy to pass tokens in a workflow (secrets.GITHUB_TOKEN), is it fine leaving it as is? Will help save a lot of error handling code.
  • Should I be worried about people messing up the file path? Technically they can write to non-SVG files and random paths. If the path is not allowed, then GitHub itself will give a permission denied error.
  • Currently if you try to generate multiple cards (lang card, stat card, etc), the action might mess up since the push workflow I've included in the example rewrites the branch each time. So only one of the cards will be there finally. Instead, the example workflow uploads as artifacts and downloads to push. This still takes only a few seconds because of the small size of the SVG files, but will this be confusing for someone who tries to write their own workflow? Using a different push action like stefanzweifel/git-auto-commit-action will solve this, but that will make pushing and GH Pages deployment slower. Ultimately, the question is about which one I should include in the example workflow, since users are free to use whichever one they want.

@vercel
Copy link

vercel bot commented Feb 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
github-readme-stats ✅ Ready (Inspect) Visit Preview Feb 22, 2023 at 0:53AM (UTC)

@Zo-Bro-23 Zo-Bro-23 added enhancement New feature or request. high-priority High priority issue or PR. labels Feb 22, 2023
@Zo-Bro-23 Zo-Bro-23 linked an issue Feb 22, 2023 that may be closed by this pull request
3 tasks
@Zo-Bro-23 Zo-Bro-23 mentioned this pull request Feb 22, 2023
3 tasks
@codecov
Copy link

codecov bot commented Feb 22, 2023

Codecov Report

Base: 97.27% // Head: 97.27% // No change to project coverage 👍

Coverage data is based on head (33ad0f4) compared to base (ba7c2f8).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2537   +/-   ##
=======================================
  Coverage   97.27%   97.27%           
=======================================
  Files          24       24           
  Lines        4178     4178           
  Branches      384      384           
=======================================
  Hits         4064     4064           
  Misses        112      112           
  Partials        2        2           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- `options` - `REQUIRED` - Any customization options. More below. (This is the same as the query string you would pass to the API if using the community deployment)
- `card` - The type of card to generate. `repo`, `top-langs`, or `wakatime` (leave empty for stats card).
- `path` - Output path for SVG file. Relative path; include filename with `.svg`.
- `fetch_multipage` - Experimental feature that gives more accurate stats if you have >100 repos.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed on discord, this can be removed and enabled by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will implement that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool thanks! People can still disable it by changing the environment variable. This variable has to be documented in the GRS readme. We can do that later.

@@ -115,7 +242,7 @@ You can pass a query parameter `&hide=` to hide any specific stats with comma-se
> Options: `&hide=stars,commits,prs,issues,contribs`

```md
![Anurag's GitHub stats](https://github-readme-stats.vercel.app/api?username=anuraghazra&hide=contribs,prs)
?username=anuraghazra&hide=contribs,prs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove the full markdown code? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When using the GitHub action, you don't give options through the URL. Instead you give the ?param=something&... options as an input for the action. Having the markdown code will make it confusing for those using the action. Instead, I've included instructions at the top on how to customize the card for the action version and the hosted version, and I'm just mentioning the custom options below. This also makes the documentation cleaner in my opinion. No need to keep repeating the markdown code if it's already specified at the top.

Copy link
Collaborator

@rickstaa rickstaa Feb 23, 2023

Choose a reason for hiding this comment

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

I already suspected that. I prefer having the full card generation code in GRS and a note in the GitHub action about this. I am afraid many beginners or lazy people will create new issues and discussions. We can document the difference in the action README. If you want, you can even add a warning/Note to make it more clear:

Warning
This is a warning.

> **Warning**
> This is a warning.

Note
This is a warning.

> **Note**
> This is a warning.

Anyway, if @anuraghazra thinks your change is fine, I'm also okay with it. 👍🏻

@@ -199,8 +326,8 @@ You can use the `bg_color` parameter to make any of [the available themes](./the
You can use [GitHub's theme context](https://github.blog/changelog/2021-11-24-specify-theme-context-for-images-in-markdown/) tags to switch the theme based on the user GitHub theme automatically. This is done by appending `#gh-dark-mode-only` or `#gh-light-mode-only` to the end of an image URL. This tag will define whether the image specified in the markdown is only shown to viewers using a light or a dark GitHub theme:

```md
[![Anurag's GitHub stats-Dark](https://github-readme-stats.vercel.app/api?username=anuraghazra&show_icons=true&theme=dark#gh-dark-mode-only)](https://github.com/anuraghazra/github-readme-stats#gh-dark-mode-only)
[![Anurag's GitHub stats-Light](https://github-readme-stats.vercel.app/api?username=anuraghazra&show_icons=true&theme=default#gh-light-mode-only)](https://github.com/anuraghazra/github-readme-stats#gh-light-mode-only)
[![Anurag's GitHub stats-Dark](https://DARK_SVG_URL/#gh-dark-mode-only)](https://github.com/anuraghazra/github-readme-stats#gh-dark-mode-only)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is temporary right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the file path is an input, the filename can be whatever the user chooses. Moreover, we directly do not push to any branch. Depending on which action they use to push, the raw url of the svg may be different. Finally, GitHub raw usercontent is blocked by Jio, which is one of the largest ISPs in India. This means that many, me included, can't view GitHub raw files. Initially I built my own proxy, but now I just use GitHub Pages. Either one will change the url. I've included instructions on how to find this URL above, and they just have to replace SVG_URL in the sample code with that url.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I think I agree with you on this point. If we clearly describe the <DARK_SVG_URL> and <SVG_URL> above it should be fine.

BTW if you would love to make the documentation more clear, I think you will love the PR I and @mezzode did in #2242.

readme.md Show resolved Hide resolved
Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

See comments.

@Zo-Bro-23
Copy link
Collaborator Author

See comments.

Thank you for your comments. I've replied to them. Could you also please answer the questions I posted with my initial comment? Some of them have been answered by your review comments (you can ignore them), but others persist. Thanks!

@rickstaa
Copy link
Collaborator

rickstaa commented Feb 23, 2023

Thank you for your comments. I've replied to them. Could you also please answer the questions I posted with my initial comment? Some of them have been answered by your review comments (you can ignore them), but others persist. Thanks!

Of course! I just answered them! You can mark them as resolved if you want. Let me know if you have any other questions.

@bunningss
Copy link

good stuff

@rickstaa
Copy link
Collaborator

rickstaa commented Feb 23, 2023

good stuff

Yeah right! I have been suggesting this since #2179. I am thrilled and thankful that @Zo-Bro-23 completed #2179🙏🏻. It will make the cards faster and more accurate and remove downtime.

@rickstaa
Copy link
Collaborator

@Zo-Bro-23 I finished all my tests and left the comments on the GRS action repo. 👍🏻 It even works with dynamic themes (see https://github.com/rickstaa/rickstaa/actions/runs/4250642780). @anuraghazra I think we can migrate to an organization structure; review #2473, merge #2537 and release the action 🚀.

@rickstaa rickstaa removed their assignment Mar 5, 2023
@Zo-Bro-23 Zo-Bro-23 mentioned this pull request Mar 21, 2023
@ofek
Copy link

ofek commented Mar 27, 2023

Can the action be used now?

@Zo-Bro-23
Copy link
Collaborator Author

Can the action be used now?

It can be used, but I'm working on writing the proper documentation and creating the required tests. I would not recommend using it until #2473 is merged.

@justinvforvendetta
Copy link

justinvforvendetta commented Jul 18, 2024

@Zo-Bro-23 shouldnt you add documentation to the grs repository README directly as well? (https://github.com/Zo-Bro-23/grs-action) when/if people find it thru the marketplace, there is no documentation for it there, other than the title. just my advice. also, great work! =]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. high-priority High priority issue or PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GRS action
5 participants