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

support using release or tag name in [GitHub] Release version badge #7075

Merged
merged 3 commits into from
Oct 14, 2021
Merged

support using release or tag name in [GitHub] Release version badge #7075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33d6e1c
feat: support using release or tag name in GitHubRelease version badge
calebcartwright Sep 26, 2021
9078a35
add comment to query param test
calebcartwright Oct 14, 2021
b2c1aff
Merge branch 'master' into github-release-name
repo-ranger[bot] Oct 14, 2021
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
1 change: 1 addition & 0 deletions services/github/github-common-release.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const releaseInfoSchema = Joi.object({
})
.required(),
tag_name: Joi.string().required(),
name: Joi.string().allow(null),
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
prerelease: Joi.boolean().required(),
}).required()

Expand Down
51 changes: 44 additions & 7 deletions services/github/github-release.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Joi from 'joi'
import { addv } from '../text-formatters.js'
import { version as versionColor } from '../color-formatters.js'
import { redirector } from '../index.js'
Expand All @@ -8,19 +9,23 @@ import {
} from './github-common-release.js'
import { documentation } from './github-helpers.js'

const extendedQueryParamSchema = Joi.object({
display_name: Joi.string().valid('tag', 'release').default('tag'),
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
})

class GithubRelease extends GithubAuthV3Service {
static category = 'version'
static route = {
base: 'github/v/release',
pattern: ':user/:repo',
queryParamSchema,
queryParamSchema: queryParamSchema.concat(extendedQueryParamSchema),
}

static examples = [
{
title: 'GitHub release (latest by date)',
namedParams: { user: 'expressjs', repo: 'express' },
queryParams: {},
queryParams: { display_name: 'tag' },
staticPreview: this.render({
version: 'v4.16.4',
sort: 'date',
Expand All @@ -31,7 +36,7 @@ class GithubRelease extends GithubAuthV3Service {
{
title: 'GitHub release (latest by date including pre-releases)',
namedParams: { user: 'expressjs', repo: 'express' },
queryParams: { include_prereleases: null },
queryParams: { include_prereleases: null, display_name: 'tag' },
staticPreview: this.render({
version: 'v5.0.0-alpha.7',
sort: 'date',
Expand All @@ -42,7 +47,7 @@ class GithubRelease extends GithubAuthV3Service {
{
title: 'GitHub release (latest SemVer)',
namedParams: { user: 'expressjs', repo: 'express' },
queryParams: { sort: 'semver' },
queryParams: { sort: 'semver', display_name: 'tag' },
staticPreview: this.render({
version: 'v4.16.4',
sort: 'semver',
Expand All @@ -53,14 +58,33 @@ class GithubRelease extends GithubAuthV3Service {
{
title: 'GitHub release (latest SemVer including pre-releases)',
namedParams: { user: 'expressjs', repo: 'express' },
queryParams: { sort: 'semver', include_prereleases: null },
queryParams: {
sort: 'semver',
include_prereleases: null,
display_name: 'tag',
},
staticPreview: this.render({
version: 'v5.0.0-alpha.7',
sort: 'semver',
isPrerelease: true,
}),
documentation,
},
{
title: 'GitHub release (release name instead of tag name)',
namedParams: { user: 'gooddata', repo: 'gooddata-java' },
queryParams: {
sort: 'date',
include_prereleases: null,
display_name: 'release',
},
staticPreview: this.render({
version: '3.7.0+api3',
sort: 'date',
isPrerelease: true,
}),
documentation,
},
]

static defaultBadgeData = { label: 'release', namedLogo: 'github' }
Expand All @@ -72,16 +96,29 @@ class GithubRelease extends GithubAuthV3Service {
return { message: addv(version), color }
}

static transform(latestRelease, display) {
const { name, tag_name: tagName, prerelease: isPrerelease } = latestRelease
if (display === 'tag') {
return { isPrerelease, version: tagName }
}

return { version: name || tagName, isPrerelease }
}

async handle({ user, repo }, queryParams) {
const latestRelease = await fetchLatestRelease(
this,
{ user, repo },
queryParams
)
const { version, isPrerelease } = this.constructor.transform(
latestRelease,
queryParams.display_name
)
return this.constructor.render({
version: latestRelease.tag_name,
version,
sort: queryParams.sort,
isPrerelease: latestRelease.prerelease,
isPrerelease,
})
}
}
Expand Down
25 changes: 25 additions & 0 deletions services/github/github-release.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { test, given } from 'sazerac'
import { GithubRelease } from './github-release.service.js'

describe('GithubRelease', function () {
test(GithubRelease.transform, () => {
given({ name: null, tag_name: '0.1.2', prerelease: true }, 'tag').expect({
version: '0.1.2',
isPrerelease: true,
})
given(
{ name: null, tag_name: '0.1.3', prerelease: true },
'release'
).expect({
version: '0.1.3',
isPrerelease: true,
})
given(
{ name: 'fun name', tag_name: '1.0.0', prerelease: false },
'release'
).expect({
version: 'fun name',
isPrerelease: false,
})
})
})
6 changes: 6 additions & 0 deletions services/github/github-release.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ t.create('Prerelease')
color: Joi.string().allow('blue', 'orange').required(),
})

// basic query parameter testing. application of param in transform
// logic is tested via unit tests in github-release.spec.js
t.create('Release (release name instead of tag name)')
.get('/v/release/expressjs/express.json?display_name=release')
.expectBadge({ label: 'release', message: isSemver, color: 'blue' })
Copy link
Member

Choose a reason for hiding this comment

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

I can see we've got the transform logic already under test in the .spec file. This test is kinda odd though because we're running it on a repo that doesn't use named releases so all we're really testing is that display_name=release is a valid param. It seems like a test I'll read in a year's time and be confused by anyway :) We could switch this to testing against a repo that uses named releases, although it is possibly then brittle in future. Another option would be to mock it e.g:

t.create('Release (custom release name)')
  .get('/v/release/expressjs/express.json')
  .intercept(nock =>
    nock('https://api.github.com')
      .get('/repos/expressjs/express/releases/latest')
      .reply(200, {
        assets: [],
        name: 'custom release name',
        tag_name: '1.0.0',
        prerelease: false,
      })
  )
  .expectBadge({
    label: 'release',
    message: 'custom release name',
    color: 'blue',
  })

Copy link
Member Author

Choose a reason for hiding this comment

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

so all we're really testing is that display_name=release is a valid param

Yup, precisely. I'm not opposed to adding another test, but feel like it's a fairly common pattern to tack on a test that simply covers the query param when we've got other unit tests covering the associated logic.

Would you still like to see a new test or perhaps even a comment to the effect of "this is just testing the query param..." etc.?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah maybe just a comment explaining this would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

added in 9078a35


t.create('Release (No releases)')
.get('/v/release/badges/daily-tests.json')
.expectBadge({ label: 'release', message: 'no releases or repo not found' })
Expand Down