-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
add support for [GitHubWorkflowStatus] (Actions) using BaseSvgScraping service implementation #3898
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
Conversation
|
The Heroku review apps seem to no longer like me 😄 Error: Cannot find module 'is-css-color'
Require stack:
- /tmp/build_c23bc303820126400ec74975c33f173e/gh-badges/lib/color.js
- /tmp/build_c23bc303820126400ec74975c33f173e/lib/logos.js
- /tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/coalesce-badge.js
- /tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/base.js
- /tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/loader.js
- /tmp/build_c23bc303820126400ec74975c33f173e/scripts/export-service-definitions-cli.js
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:713:15)
at Function.Module._load (internal/modules/cjs/loader.js:618:27)
at Module.require (internal/modules/cjs/loader.js:771:19)
at require (internal/modules/cjs/helpers.js:68:18)
at Object.<anonymous> (/tmp/build_c23bc303820126400ec74975c33f173e/gh-badges/lib/color.js:3:20)
at Module._compile (internal/modules/cjs/loader.js:868:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:879:10)
at Module.load (internal/modules/cjs/loader.js:731:32)
at Function.Module._load (internal/modules/cjs/loader.js:644:12)
at Module.require (internal/modules/cjs/loader.js:771:19) {
code: 'MODULE_NOT_FOUND',
requireStack: [
'/tmp/build_c23bc303820126400ec74975c33f173e/gh-badges/lib/color.js',
'/tmp/build_c23bc303820126400ec74975c33f173e/lib/logos.js',
'/tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/coalesce-badge.js',
'/tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/base.js',
'/tmp/build_c23bc303820126400ec74975c33f173e/core/base-service/loader.js',
'/tmp/build_c23bc303820126400ec74975c33f173e/scripts/export-service-definitions-cli.js'
]
}
npm ERR! code ELIFECYCLE |
In terms of a validator, the scope of 'actions' is much larger than a CI service. They can be used to deploy, build/publish packages, send notifications, etc ( see https://github.com/marketplace?type=actions for examples ) so I'd expect we'd see a much wider range of values here. Beyond that, the code for this is fine. My main reservation with this is relying on this endpoint given we don't have any docs from GitHub acknowledging it exists. At least we know the beta API endpoint is documented as being in beta. Anyone else have any thoughts on this? |
I don’t feel strongly, though I’d lean toward using the API. We have a lot of quota so that doesn’t seem like a main concern. |
I'd always prefer using an API 😄 I had intended to open up an implementation using the API as well for comparison & discussion but got distracted.
I don't disagree, and I'm definitely not pushing for the SvgScraping implementation (I'd vote for the API as well). My thought process was that long term we may have some questions around quota (for example what the GraphQL API impact may or may not be), and whether Shields being able to provide a badge for GitHub data without hitting the quota would be a plus. Especially since I could see GH Actions/Worklow badges quickly becoming one of our most heavily used badges, and we'd presumably have some set of folks switching from Circle/Travis/etc. badges to these. IMO it is a plus, but a pretty small one that wouldn't change my vote.
I realized that the upstream badge GitHub is providing and used here is really more of a Workflow Status badge vs. individual actions, so the PR title/Service class name/etc. here should be updated to be more precise. It would not surprise me at all if there are additional states that may be involved here, but I would note that a GitHub workflow in that context really isn't any different than Jenkins, Azure DevOps, Circle, etc. Those can/are used to do many of the same sort of steps with orchestration, but at the end of the day the top level pipeline/workflow/etc. still has some final status that conveys a more wholistic pass/fail/etc. state, and so accordingly do our badges. |
Now that GH Actions has come out of beta, the native status badges support an optional query param for branch specification (which was the biggest feature gap with this approach). I'll check to see if the CheckSuites API now provides a way to query/filter based on workflow name now too. If not, I think we'll need to go with this approach so that users can specify both the workflow and branch names |
In the spirit of not allowing perfection to be the enemy of the good (enough), I think we should go ahead with this approach to support GitHub Actions/Workflows. I know there's a collective preference for APIs over badge scraping (myself included), but I've still not found away to use the GitHub Checks Suite API (which is much broader and more generic than GH Actions) that supports getting the status of a specific workflow by workflow-name. If/when we discover an alternative way to to get the data, we can always swap out our implementation at that time. However, there's a ton of interest for GH Actions and IMHO we should proceed with delivering this badge to the communtiy. With GitHub recently adding support for branch querying, the only outstanding item (AFAIK) with this PR is a question/concern @chris48s raised regarding using the While I agree that GH actions can/are used for all sorts of things, the badge being scraped here is the
Based on the text from those docs, as well as what I've seen, I'm fairly confident that this status badge will follow the standard On the off chance that GH's native status badge can contain wildly different status values (for example a test summary message like |
Here's an example of a repo with multiple-workflows that illustrate why it's important for users to be able to specify both the branch and workflow they want a badge for https://github.com/rust-lang/mdBook/tree/master/.github/workflows https://shields-staging-pr-3898.herokuapp.com/github/actions/rust-lang/mdBook/CI/master https://shields-staging-pr-3898.herokuapp.com/github/actions/rust-lang/mdBook/Deploy/master https://shields-staging-pr-3898.herokuapp.com/github/actions/rust-lang/mdBook/Deploy/gh-pages And here's an even better example repo over in the Apache org that really emphasizes why workflow name is critical 😄 |
I can't wait this! Thanks for your effort @calebcartwright! |
I'm also leaning towards renaming the route/class/etc. to refer to this as That's because there can be multiple jobs within a workflow, and it's highly likely IMO that we'll want to support job status badges in the future ( Thoughts? |
I would go with the shorter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Will re-review promptly when the aforementioned path name change is implemented 👍
.get('/actions/toolkit/Main%20workflow.json') | ||
.expectBadge({ | ||
label: 'build', | ||
message: isBuildStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth adding the no status
case to avoid flaky tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
👏 |
Big thanks, @calebcartwright 🎉 |
Just a heads up for anyone following this thread.. it's going to be a bit longer before this is deployed and available |
Thanks for getting this one over the line. Sorry I didn't get a chance to post earlier, but now that GitHub are documenting this exists, I think its fine to rely on it. Also agree on validation schema 👍 As a slight tangent, it is interesting to see GH starting to serve and support their own badges. I wonder if we will see more popping up. One to keep an eye on 👀 |
I stand corrected 😆 this is live! Thanks @paulmelnikow for getting this shipped so quickly 🎉 Some examples: And one with all sorts of Shields-customization goodness |
WOW! Thank you @paulmelnikow! |
GitHub Actions now have a fresh new API, might be useful to get the workflow names and statuses without scrapping: https://developer.github.com/v3/actions/ |
Thanks for letting us know @arcanis! |
Refs #2574 - I'm not sure if this is the way we want to go, but opening for discussion.
I also don't know enough about GitHub Actions to know the full scope of values we may see in the badges, and whetherEdit - This is scraping GH's workflow status badge which AFAICT uses typical result statuses we useisBuildStatus
will sufficiently cover thoseisBuildStatus
for