add support for [GitHubActions] via API #3913

calebcartwright · 2019-08-25T18:38:27Z

Refs #2574. An alternative approach to #3898 that leverages the GH v3 Check Suites API instead of badge scraping the native GH Workflow result badge.

Although I still prefer this approach to #3898, there is one downside in instances where a repo has more than one Workflow running on a given ref (branch, etc.) vs. #3898.

In such a scenario, the response from the check-suites will include multiple records for each Workflow but there's no way to associate the record in the API response to a named Workflow, at least not that I've been able to find. For example, https://api.github.com/repos/actions/setup-python/commits/master/check-suites

The badge scraping approach in #3898 does allow the user to explicitly specify which Workflow they want by workflow name, but it only works on the master branch. The API approach allows the user to pick the branch/ref, but not the specific workflow by name

calebcartwright · 2019-08-25T18:39:44Z

services/github/github-api-provider.js

@@ -185,7 +185,8 @@ class GithubApiProvider {
        baseUrl,
        headers: {
          'User-Agent': 'Shields.io',
-          Accept: 'application/vnd.github.v3+json',
+          Accept:
+            'application/vnd.github.v3+json, application/vnd.github.antiope-preview+json',
          Authorization: `token ${tokenString}`,
        },


Was the overriding of provided options deliberate here? I had to add the alternative media type required for this API here as it wasn't working from the service class due to the merge order

I'm sure there is an open issue about the way we're merging options, but I can't find it for the life of me.

We often merge in the other order e.g:

shields/core/base-service/base-json.js

Lines 43 to 46 in c9cd77a

const mergedOptions = {

...{ headers: { Accept: 'application/json' } },

...options,

}

but the downside of this is if we want to pass an extra header, we have to re-add Accept: 'application/json' (because the spread operation doesn't nest, if you see what I mean)

I suspect the reason we've done it in the other order here is so that if we pass any header, its not going to overwrite the Authorization header.

In general, we need a more sophisticated merge starategy for these options objects.

shields-ci · 2019-08-25T18:40:47Z

	Messages
📖	✨ Thanks for your contribution to Shields, @calebcartwright!

Generated by 🚫 dangerJS against 9aa9b81

calebcartwright · 2019-08-25T18:43:03Z

services/github/github-actions.service.js

+  check_suites: Joi.array()
+    .items(
+      Joi.object({
+        status: Joi.string().required(),


We could probably try to use a stricter validation here for status and conclusion, although I've intentionally kept these at Joi.string(). AFAICT, GitHub apps can update the list of possible status values that they use, which would flow through here (since this API returns all Check Suites, not just GH Actions Check Suites).

I believe that if we tried to restrict the allowed strings for these fields that we'd inevitably end up with users receiving "invalid response data" badges

yes I think GH Actions is sufficiently flexible that its hard to reliably require something more restrictive here

calebcartwright · 2019-08-25T18:53:12Z

Failing service tests were other unrelated GitHub badge tests (and didn't fail in services@node-latest) so I'm going to update the PR title to only run the tests for this new service

PyvesB

This overall looks good to me. I've left a few questions and suggestions inline. 👍

I'm more in favour of this API approach rather than the badge scraping one suggested in #3898. We've often had problems with badge scrapping in the past where the badge format was suddenly changed and broke our implementation. APIs are less likely to change (apart for preview features though) and hopefully they will expand the information to include things such as workflow names.

PyvesB · 2019-11-24T11:06:13Z

services/github/github-actions.service.js

+  static render({ status, conclusion }) {
+    if (status !== 'completed') {
+      return {
+        message: 'in progress',


in progress should probably be added to the list of otherStatuses in build-status.js. The test 'valid repo and ref' may occasionally fail otherwise. I would also suggest using renderBuildStatusBadge for the 'in progress' value as well: it won't change anything with the current implementation, but if ever we decide to change the colour or something else, this build badge will automatically be covered as well. 😉

PyvesB · 2019-11-24T11:07:49Z

services/github/github-api-provider.js

@@ -185,7 +185,8 @@ class GithubApiProvider {
        baseUrl,
        headers: {
          'User-Agent': 'Shields.io',
-          Accept: 'application/vnd.github.v3+json',
+          Accept:
+            'application/vnd.github.v3+json, application/vnd.github.antiope-preview+json',


Hmm... May be worth documenting what the second accept value is for.

PyvesB · 2019-11-24T11:10:24Z

services/github/github-actions.service.js

+  }
+
+  transform(json) {
+    if (json.total_count === 0) {


Do we need this extra check? Wouldn't the if (!actionsSuite) get us covered as well?

calebcartwright · 2019-11-25T00:06:35Z

Thanks for reviewing @PyvesB!

I'm most likely going to just close this PR. I understand (and agree with) the preference for the API over badge scraping, but this approach simply isn't viable without the ability to utilize the workflow name (it's as core as being able to specify the GH owner and repo).

The badge scraping approach is the only option that's available right now, and I've updated that PR with more detail in #3898 (comment).

The Check Suite and Status APIs are not specific Actions/Workflow, but perhaps GitHub will eventually roll out Actions/Workflow APIs that we could switch to down the road. However, IMO we should proceed with the badge scraping approach as there is a huge community demand for this badge that we can meet, today, with that approach.

PyvesB · 2019-11-25T07:48:52Z

Could we simply ask for the workflow name feature on the GitHub community forums? GitHub staff have been quite good at solving issues and responding to user queries regarding GitHub actions. I will look at the other PR in the evening.

PyvesB · 2019-11-25T21:05:44Z

This feature request may well do the trick when implemented: https://github.community/t5/GitHub-API-Development-and/How-to-retrieve-the-full-list-of-Actions-workflow-runs-shown-on/m-p/39171

calebcartwright · 2019-11-29T21:34:10Z

Closing this as #3898 has been merged and deployed

feat: add GitHub Actions support via API

4768ce9

calebcartwright added the service-badge New or updated service badge label Aug 25, 2019

shields-cd temporarily deployed to shields-staging-pr-3913 August 25, 2019 18:38 Inactive

calebcartwright commented Aug 25, 2019

View reviewed changes

calebcartwright changed the title ~~add support for [GitHub] Actions via API~~ add support for [GitHubActions] via API Aug 25, 2019

Merge branch 'master' into gh-actions-api

30c566e

shields-cd temporarily deployed to shields-staging-pr-3913 August 25, 2019 18:53 Inactive

calebcartwright mentioned this pull request Aug 25, 2019

Badge request: GitHub actions #2574

Closed

shields-cd temporarily deployed to shields-staging-pr-3913 September 1, 2019 20:50 Inactive

shields-cd deployed to shields-staging-pr-3913 September 8, 2019 20:54 View deployment

Merge branch 'master' into gh-actions-api

9aa9b81

PyvesB reviewed Nov 24, 2019

View reviewed changes

calebcartwright closed this Nov 29, 2019

calebcartwright deleted the gh-actions-api branch November 29, 2019 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for [GitHubActions] via API #3913

add support for [GitHubActions] via API #3913

calebcartwright commented Aug 25, 2019 •

edited

Loading

calebcartwright Aug 25, 2019

chris48s Aug 25, 2019

shields-ci commented Aug 25, 2019 •

edited

Loading

calebcartwright Aug 25, 2019

chris48s Aug 25, 2019

calebcartwright commented Aug 25, 2019

PyvesB left a comment

PyvesB Nov 24, 2019

PyvesB Nov 24, 2019

PyvesB Nov 24, 2019

calebcartwright commented Nov 25, 2019

PyvesB commented Nov 25, 2019

PyvesB commented Nov 25, 2019

calebcartwright commented Nov 29, 2019

	const mergedOptions = {
	...{ headers: { Accept: 'application/json' } },
	...options,
	}

add support for [GitHubActions] via API #3913

add support for [GitHubActions] via API #3913

Conversation

calebcartwright commented Aug 25, 2019 • edited Loading

calebcartwright Aug 25, 2019

Choose a reason for hiding this comment

chris48s Aug 25, 2019

Choose a reason for hiding this comment

shields-ci commented Aug 25, 2019 • edited Loading

calebcartwright Aug 25, 2019

Choose a reason for hiding this comment

chris48s Aug 25, 2019

Choose a reason for hiding this comment

calebcartwright commented Aug 25, 2019

PyvesB left a comment

Choose a reason for hiding this comment

PyvesB Nov 24, 2019

Choose a reason for hiding this comment

PyvesB Nov 24, 2019

Choose a reason for hiding this comment

PyvesB Nov 24, 2019

Choose a reason for hiding this comment

calebcartwright commented Nov 25, 2019

PyvesB commented Nov 25, 2019

PyvesB commented Nov 25, 2019

calebcartwright commented Nov 29, 2019

calebcartwright commented Aug 25, 2019 •

edited

Loading

shields-ci commented Aug 25, 2019 •

edited

Loading