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.

Sign up for GitHub

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

Jump to bottom

switch from github workflows to github actions workflows; test [githubactionsworkflowstatus githubworkflowstatus] #8475

Merged
merged 5 commits into from
Dec 15, 2022
Merged

switch from github workflows to github actions workflows; test [githubactionsworkflowstatus githubworkflowstatus] #8475

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
808dfec
switch from github workflows to github actions workflows
chris48s Oct 3, 2022
7ea7cf5
update github actions workflow badge to use api
chris48s Oct 17, 2022
de39617
add test case for missing branch param
chris48s Oct 17, 2022
e0b7dbd
custom deprecation message
chris48s Dec 1, 2022
7e7d006
Merge branch 'master' into 8146-gitlab-workflows
chris48s Dec 15, 2022
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
101 changes: 101 additions & 0 deletions services/github/github-actions-workflow-status.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import Joi from 'joi'
import { isBuildStatus, renderBuildStatusBadge } from '../build-status.js'
import { NotFound } from '../index.js'
import { GithubAuthV3Service } from './github-auth-service.js'
import { documentation, errorMessagesFor } from './github-helpers.js'

const schema = Joi.object({
workflow_runs: Joi.array()
.items(
Joi.object({
conclusion: Joi.alternatives()
.try(isBuildStatus, Joi.equal('no status'))
.required(),
})
)
.required()
.min(0)
.max(1),
}).required()

const queryParamSchema = Joi.object({
event: Joi.string(),
branch: 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.

I've made branch a required param for this badge. Not my favourite solution, but workflow can contain a / and branch=HEAD doesn't work so we can't make the default "whatever the default branch is" in one request, so required query param it is. Lets lay that marker down while we're making a breaking change.

I don't think "latest workflow status across all branches" is a sensible default.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know off hand if this has to explicitly be a branch or will the upstream endpoint accept any valid ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

The docs say

branch string

Returns workflow runs associated with a branch. Use the name of the branch of the push.

Anecdotally, I tried some calls with a tag and a commit hash (which I expected would return a valid result if this could accept any ref). They returned 0 results.

}).required()

const keywords = ['action', 'actions']

export default class GithubActionsWorkflowStatus extends GithubAuthV3Service {
static category = 'build'

static route = {
base: 'github/actions/workflow/status',
pattern: ':user/:repo/:workflow+',
chris48s marked this conversation as resolved.
Show resolved Hide resolved
queryParamSchema,
}

static examples = [
{
title: 'GitHub Workflow Status',
namedParams: {
user: 'actions',
repo: 'toolkit',
workflow: 'unit-tests.yml',
},
queryParams: {
branch: 'main',
},
staticPreview: renderBuildStatusBadge({
status: 'passing',
}),
documentation,
keywords,
},
{
title: 'GitHub Workflow Status (with event)',
namedParams: {
user: 'actions',
repo: 'toolkit',
workflow: 'unit-tests.yml',
},
queryParams: {
event: 'push',
branch: 'main',
},
staticPreview: renderBuildStatusBadge({
status: 'passing',
}),
documentation,
keywords,
},
]

static defaultBadgeData = {
label: 'build',
}

async fetch({ user, repo, workflow, branch, event }) {
return await this._requestJson({
schema,
url: `/repos/${user}/${repo}/actions/workflows/${workflow}/runs`,
options: {
searchParams: {
branch,
event,
page: '1',
per_page: '1',
exclude_pull_requests: 'true',
Comment on lines +85 to +87
Copy link
Member Author

Choose a reason for hiding this comment

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

These params are here for performance reasons. There's huge execution time difference between this query with exclude_pull_requests=true (fast) vs exclude_pull_requests=false (slow)

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Definitely understand the performance angle, but curious if there's any functional impacts? Am I correctly interpreting this as meaning that runs triggered by a pull request would be excluded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. Passing this means the pull_requests array in the JSON response is not populated, but we aren't using any values from it so it doesn't really matter here. We can still get the details of a workflow that is triggered by a PR. For example, if I call https://api.github.com/repos/badges/shields/actions/workflows/enforce-dependency-review.yml/runs?branch=frontend-gha&exclude_pull_requests=true (the enforce-dependency-review.yml workflow on our repo only runs on PR triggers), we get a response with all the info we need for a badge. Just the pull_requests array is empty.

},
},
errorMessages: errorMessagesFor('repo or workflow not found'),
})
}

async handle({ user, repo, workflow }, { branch, event }) {
const data = await this.fetch({ user, repo, workflow, branch, event })
if (data.workflow_runs.length === 0) {
throw new NotFound({ prettyMessage: 'branch or event not found' })
}
return renderBuildStatusBadge({ status: data.workflow_runs[0].conclusion })
}
}
52 changes: 52 additions & 0 deletions services/github/github-actions-workflow-status.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import Joi from 'joi'
import { isBuildStatus } from '../build-status.js'
import { createServiceTester } from '../tester.js'
export const t = await createServiceTester()

const isWorkflowStatus = Joi.alternatives()
.try(isBuildStatus, Joi.equal('no status'))
.required()

t.create('nonexistent repo')
.get('/badges/shields-fakeness/fake.yml.json?branch=main')
.expectBadge({
label: 'build',
message: 'repo or workflow not found',
})

t.create('nonexistent workflow')
.get('/actions/toolkit/not-a-real-workflow.yml.json?branch=main')
.expectBadge({
label: 'build',
message: 'repo or workflow not found',
})

t.create('nonexistent branch')
.get('/actions/toolkit/unit-tests.yml.json?branch=not-a-real-branch')
.expectBadge({
label: 'build',
message: 'branch or event not found',
})

t.create('nonexistent event')
.get(
'/actions/toolkit/unit-tests.yml.json?branch=main&event=not-a-real-event'
)
.expectBadge({
label: 'build',
message: 'branch or event not found',
})

t.create('valid workflow')
.get('/actions/toolkit/unit-tests.yml.json?branch=main')
.expectBadge({
label: 'build',
message: isWorkflowStatus,
})

t.create('valid workflow (with event)')
.get('/actions/toolkit/unit-tests.yml.json?branch=main&event=push')
.expectBadge({
label: 'build',
message: isWorkflowStatus,
})
107 changes: 9 additions & 98 deletions services/github/github-workflow-status.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,100 +1,11 @@
import Joi from 'joi'
import { isBuildStatus, renderBuildStatusBadge } from '../build-status.js'
import { BaseSvgScrapingService } from '../index.js'
import { documentation } from './github-helpers.js'
import { deprecatedService } from '../index.js'

const schema = Joi.object({
message: Joi.alternatives()
.try(isBuildStatus, Joi.equal('no status'))
.required(),
}).required()

const queryParamSchema = Joi.object({
event: Joi.string(),
}).required()

const keywords = ['action', 'actions']

export default class GithubWorkflowStatus extends BaseSvgScrapingService {
static category = 'build'

static route = {
export default deprecatedService({
Copy link
Member Author

Choose a reason for hiding this comment

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

Normally when we deprecate a badge we are just getting rid of it completely. Often the upstream service has been retired, or whatever. If we can provide a migration path from old to new, we use a redirect. In this case, we can't do either of those things because the :workflow param is not compatible between the two badges. Additionally, the GitHub workflow status badge is one of our most popular badges 😬 I think deprecating the old route and making people switch is the right thing to do because the behaviour of the existing badge is unexpected but its going to generate some support requests/anger/confusion/etc. Before we merge this I definitely want to write up a pinned issue (I've not written it yet, but I will) explaining what is going on and how to migrate.

Because we've never had this situation where we're asking people to switch and we can't provide a redirect, we've never done this before, but I wonder if instead of serving build | no longer available we make the issue and serve build | github.com/badges/shields/issues/9999 (also use the link property for any <object> embeds) to help direct people towards upgrade advice. I'm somewhat conflicted as it is mainly info for maintainers and most of the people who will see it are going to be end users who can't do anything about it.

Copy link
Member

@calebcartwright calebcartwright Nov 30, 2022 •
edited by chris48s
Loading

Choose a reason for hiding this comment

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

but I wonder if instead of serving build | no longer available we make the issue and serve build | github.com//issues/9999 (also use the link property for any <object> embeds) to help direct people towards upgrade advice. I'm somewhat conflicted as it is mainly info for maintainers and most of the people who will see it are going to be end users who can't do anything about it.

I'd recommend going with your issue link suggestion. Completely agreed that 99% of those that see the badge won't be able to do anything about it, but having a no longer available message instead doesn't really improve their experience one way or another and I think having the issue link only increases awareness; some subset of those users that see the issue-link badge in someone else's project will have their own such badges in their respective repos that they do need to take action on but may still be unaware

Copy link
Member Author

Choose a reason for hiding this comment

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

OK. I have made a placeholder issue #8671 so we have a known URL we can link to, and I've done that in de39617 . This should allow us to approve the code changes. I will work on writing up the issue explaining why we are making the change, what users need to do about it, etc before I deploy it.

category: 'build',
route: {
base: 'github/workflow/status',
pattern: ':user/:repo/:workflow/:branch*',
queryParamSchema,
}

static examples = [
{
title: 'GitHub Workflow Status',
pattern: ':user/:repo/:workflow',
namedParams: {
user: 'actions',
repo: 'toolkit',
workflow: 'toolkit-unit-tests',
},
staticPreview: renderBuildStatusBadge({
status: 'passing',
}),
documentation,
keywords,
},
{
title: 'GitHub Workflow Status (branch)',
pattern: ':user/:repo/:workflow/:branch',
namedParams: {
user: 'actions',
repo: 'toolkit',
workflow: 'toolkit-unit-tests',
branch: 'master',
},
staticPreview: renderBuildStatusBadge({
status: 'passing',
}),
documentation,
keywords,
},
{
title: 'GitHub Workflow Status (event)',
pattern: ':user/:repo/:workflow',
namedParams: {
user: 'actions',
repo: 'toolkit',
workflow: 'toolkit-unit-tests',
},
queryParams: {
event: 'push',
},
staticPreview: renderBuildStatusBadge({
status: 'passing',
}),
documentation,
keywords,
},
]

static defaultBadgeData = {
label: 'build',
}

async fetch({ user, repo, workflow, branch, event }) {
const { message: status } = await this._requestSvg({
schema,
url: `https://github.com/${user}/${repo}/workflows/${encodeURIComponent(
workflow
)}/badge.svg`,
options: { searchParams: { branch, event } },
valueMatcher: />([^<>]+)<\/tspan><\/text><\/g><path/,
errorMessages: {
404: 'repo, branch, or workflow not found',
},
})

return { status }
}

async handle({ user, repo, workflow, branch }, { event }) {
const { status } = await this.fetch({ user, repo, workflow, branch, event })
return renderBuildStatusBadge({ status })
}
}
pattern: ':various+',
},
label: 'build',
dateAdded: new Date('2022-10-03'),
})
33 changes: 16 additions & 17 deletions services/github/github-workflow-status.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,43 +1,42 @@
import Joi from 'joi'
import { isBuildStatus } from '../build-status.js'
import { createServiceTester } from '../tester.js'
export const t = await createServiceTester()
import { ServiceTester } from '../tester.js'

const isWorkflowStatus = Joi.alternatives()
.try(isBuildStatus, Joi.equal('no status'))
.required()
export const t = new ServiceTester({
id: 'GithubWorkflowStatus',
title: 'Github Workflow Status',
pathPrefix: '/github/workflow/status',
})

t.create('nonexistent repo')
t.create('no longer available (previously nonexistent repo)')
.get('/badges/shields-fakeness/fake.json')
.expectBadge({
label: 'build',
message: 'repo, branch, or workflow not found',
message: 'no longer available',
})

t.create('nonexistent workflow')
t.create('no longer available (previously nonexistent workflow)')
.get('/actions/toolkit/not-a-real-workflow.json')
.expectBadge({
label: 'build',
message: 'repo, branch, or workflow not found',
message: 'no longer available',
})

t.create('valid workflow')
t.create('no longer available (previously valid workflow)')
.get('/actions/toolkit/toolkit-unit-tests.json')
.expectBadge({
label: 'build',
message: isWorkflowStatus,
message: 'no longer available',
})

t.create('valid workflow (branch)')
t.create('no longer available (previously valid workflow - branch)')
.get('/actions/toolkit/toolkit-unit-tests/master.json')
.expectBadge({
label: 'build',
message: isWorkflowStatus,
message: 'no longer available',
})

t.create('valid workflow (event)')
t.create('no longer available (previously valid workflow - event)')
.get('/actions/toolkit/toolkit-unit-tests.json?event=push')
.expectBadge({
label: 'build',
message: isWorkflowStatus,
message: 'no longer available',
})