Skip to content

add support for [GitHubActions] via API #3913

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions services/github/github-actions.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
'use strict'

const Joi = require('@hapi/joi')
const { renderBuildStatusBadge } = require('../build-status')
const { nonNegativeInteger } = require('../validators')
const { GithubAuthV3Service } = require('./github-auth-service')
const { documentation, errorMessagesFor } = require('./github-helpers')
const { NotFound } = require('..')

const schema = Joi.object({
total_count: nonNegativeInteger,
check_suites: Joi.array()
.items(
Joi.object({
status: Joi.string().required(),
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

conclusion: Joi.string()
.required()
.allow(null),
app: Joi.object({
id: nonNegativeInteger,
name: Joi.string().required(),
}).required(),
})
)
.min(0)
.required(),
}).required()

module.exports = class GitHubActions extends GithubAuthV3Service {
static get category() {
return 'build'
}

static get route() {
return {
base: 'github/actions',
pattern: ':user/:repo/:ref',
}
}

static get examples() {
return [
{
title: 'GitHub Actions',
namedParams: {
user: 'actions',
repo: 'setup-node',
ref: 'master',
},
staticPreview: renderBuildStatusBadge({ status: 'success' }),
documentation: `
You can use a branch name, tag, or commit SHA for the ref parameter.
${documentation}
`,
},
]
}

static get defaultBadgeData() {
return {
label: 'workflow',
}
}

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

Choose a reason for hiding this comment

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

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. 😉

}
}
return renderBuildStatusBadge({ status: conclusion })
}

async fetch({ user, repo, ref }) {
return this._requestJson({
url: `/repos/${user}/${repo}/commits/${ref}/check-suites`,
options: {
headers: {
Accept: 'application/vnd.github.antiope-preview+json',
},
},
schema,
errorMessages: errorMessagesFor('repo or ref not found'),
})
}

transform(json) {
if (json.total_count === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

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

throw new NotFound({ prettyMessage: 'no GitHub Actions found' })
}

const actionsSuite = json.check_suites.find(
suite => suite.app.name.toLowerCase() === 'github actions'
)
if (!actionsSuite) {
throw new NotFound({ prettyMessage: 'no GitHub Actions found' })
}

return { status: actionsSuite.status, conclusion: actionsSuite.conclusion }
}

async handle({ user, repo, ref }) {
const json = await this.fetch({ user, repo, ref })
const { status, conclusion } = this.transform(json)
return this.constructor.render({ status, conclusion })
}
}
21 changes: 21 additions & 0 deletions services/github/github-actions.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict'

const { test, given, forCases } = require('sazerac')
const GitHubActions = require('./github-actions.service')

describe('GitHubActions', function() {
test(GitHubActions.render, () => {
forCases([
given({
status: 'queued',
conclusion: null,
}),
given({
status: 'pending',
conclusion: null,
}),
]).expect({
message: 'in progress',
})
})
})
39 changes: 39 additions & 0 deletions services/github/github-actions.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict'

const { isBuildStatus } = require('../build-status')
const t = (module.exports = require('../tester').createServiceTester())

t.create('valid repo and ref')
.get('/actions/setup-node/master.json')
.expectBadge({
label: 'workflow',
message: isBuildStatus,
})

t.create('valid repo and invalid ref')
.get('/actions/setup-node/not-a-real-branch-or-tag-or-commit.json')
.expectBadge({
label: 'workflow',
message: 'repo or ref not found',
})

t.create('invalid repo')
.get('/actions/setup-node123abcdef456/master.json')
.expectBadge({
label: 'workflow',
message: 'repo or ref not found',
})

t.create('no check suites')
.get('/badges/ServerScript/master.json')
.expectBadge({
label: 'workflow',
message: 'no GitHub Actions found',
})

t.create('no GitHub Actions')
.get('/badges/shields/master.json')
.expectBadge({
label: 'workflow',
message: 'no GitHub Actions found',
})
3 changes: 2 additions & 1 deletion services/github/github-api-provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

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

Authorization: `token ${tokenString}`,
},
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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:

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.

},
Expand Down