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.

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

add GitLab Release badge, run all [GitLab] #7021

Merged
merged 8 commits into from
Oct 23, 2021
46 changes: 46 additions & 0 deletions services/gitlab/gitlab-base.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,50 @@ export default class GitLabBase extends BaseJsonService {
})
)
}

async fetchPage({ page, requestParams, schema }) {
const { res, buffer } = await this._request({
...requestParams,
...{ options: { qs: { page } } },
})

const json = this._parseJson(buffer)
const data = this.constructor._validate(json, schema)
return { res, data }
}

async fetchPaginatedArrayData({
url,
options,
schema,
errorMessages,
firstPageOnly = false,
}) {
const requestParams = this.authHelper.withBasicAuth({
url,
options: {
headers: { Accept: 'application/json' },
qs: { per_page: 100 },
...options,
},
errorMessages,
})

const {
res: { headers },
data,
} = await this.fetchPage({ page: 1, requestParams, schema })
const numberOfPages = headers['x-total-pages']

if (numberOfPages === 1 || firstPageOnly) {
return data
}

const pageData = await Promise.all(
[...Array(numberOfPages - 1).keys()].map((_, i) =>
this.fetchPage({ page: ++i + 1, requestParams, schema })
)
)
return [...data].concat(...pageData)
}
Comment on lines +31 to +64
Copy link
Member Author

Choose a reason for hiding this comment

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

GitLab's max per page value is 100, and I saw that exceeded in several cases just with the releases on the handful of projects I tested with. A cursory glance through the GitLab API docs makes it seem a very common response structure will be pages of arrays with the page keys in the response headers.

Opted to go ahead and pull that into the base class in anticipation of other APIs. Added it with relevant short circuiting logic (since GitLab doesn't seem to have a separate "latest" API endpoint that returns a single object like GitHub does in some cases), and also requesting all the pages in parallel since it's both possible to do so and preferable to 1 by 1

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

I know this is a bit of a fiddly thing to write a test for, but if we're going to be relying on this logic in a number of place it would be good to have it under test

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 service tests include a target (the GitLab repo itself) which has more than 100 releases so it is observable (albeit manually via TRACE_SERVICES). Probably some class level unit testing is possible with sufficient mocks, but feel like that's probably best investigated/implemented in a separate PR, thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a plan 👍

}
136 changes: 136 additions & 0 deletions services/gitlab/gitlab-release.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import Joi from 'joi'
import { optionalUrl } from '../validators.js'
import { latest, renderVersionBadge } from '../version.js'
import { NotFound } from '../index.js'
import GitLabBase from './gitlab-base.js'

const schema = Joi.array().items(
Joi.object({
name: Joi.string().required(),
tag_name: Joi.string().required(),
})
)

const queryParamSchema = Joi.object({
gitlab_url: optionalUrl,
include_prereleases: Joi.equal(''),
sort: Joi.string().valid('date', 'semver').default('date'),
display_name: Joi.string().valid('tag', 'release').default('tag'),
Copy link
Member Author

Choose a reason for hiding this comment

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

Figured it made sense to add this from day 0 given where we ended up with this on GitHub. I've opted to set the default to tag to be consistent with what GitHub currently is but don't have strong opinions on this one way or another.

}).required()

const namedParams = {
user: 'shields-ops-group',
repo: 'repo-test',
}

export default class GitLabRelease extends GitLabBase {
static category = 'version'

static route = {
base: 'gitlab/v/release',
pattern: ':user/:repo',
Copy link
Member

Choose a reason for hiding this comment

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

I probably should have picked this up in the GitLab tag PR review, but on Gitlab repos can be nested in subgroups e.g: https://gitlab.com/megabyte-labs/dockerfile/ci-pipeline/ansible-lint (refs #6427 ) . As we start adding a suite of GitLab badges, lets establish the convention that for new API based badges we work with this pattern and don't restirct to only the user/repo format (I think this means allowing an arbitrary number of slashes in this part of the URL). I think this also then implies that any new GitLab badges which take a branch param accept it as a query param ?branch= rather than a URL param (although this possibly depends a bit on the upstream API routes).

Copy link
Member Author

Choose a reason for hiding this comment

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

Two additional wrinkles to consider:

I almost feel like this is going to force us to have a single param where users can either go the easy route and plugin the project id, or the full project path and we'll handle the uri encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah so I'm suggesting the pattern we adopt is /:repo+ for the [group(s)]/user/repo and then we pass encodeURIComponent(repo) to the API. Then for any badges where a branch param is valid, we implement that as ?branch=name. I think this pattern of nesting in groups is quite common for gitlab repos and I reckon it is worth allowing the 'friendly' form of the name rather than allowing you to use the repo name for some repositories but requiring the ID for others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, agreed. Believe bfe7c1c should have this all sorted now, though will need to circle back to how we best apply the pattern to other badges

queryParamSchema,
}

static examples = [
{
title: 'GitLab Release (latest by date)',
namedParams,
queryParams: { sort: 'date' },
staticPreview: renderVersionBadge({ version: 'v2.0.0' }),
},
{
title: 'GitLab Release (latest by SemVer)',
namedParams,
queryParams: { sort: 'semver' },
staticPreview: renderVersionBadge({ version: 'v4.0.0' }),
},
{
title: 'GitLab Release (latest by SemVer pre-release)',
namedParams,
queryParams: {
sort: 'semver',
include_prereleases: null,
},
staticPreview: renderVersionBadge({ version: 'v5.0.0-beta.1' }),
},
{
title: 'GitLab Release (custom instance)',
namedParams: {
user: 'GNOME',
repo: 'librsvg',
},
queryParams: {
sort: 'semver',
include_prereleases: null,
gitlab_url: 'https://gitlab.gnome.org',
},
staticPreview: renderVersionBadge({ version: 'v2.51.4' }),
},
{
title: 'GitLab Release (by release name)',
namedParams: {
user: 'gitlab-org',
repo: 'gitlab',
},
queryParams: {
sort: 'semver',
include_prereleases: null,
gitlab_url: 'https://gitlab.com',
display_name: 'release',
},
staticPreview: renderVersionBadge({ version: 'GitLab 14.2' }),
},
]

static defaultBadgeData = { label: 'release' }

async fetch({ user, repo, baseUrl, isSemver }) {
// https://docs.gitlab.com/ee/api/releases/
return this.fetchPaginatedArrayData({
schema,
url: `${baseUrl}/api/v4/projects/${user}%2F${repo}/releases`,
Copy link
Member

Choose a reason for hiding this comment

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

https://docs.gitlab.com/ee/api/releases/ says the default ordering is released_at so I guess we are saying ?sort=date corresponds to released_at (as opposed to created_at). Do you have a handle on the difference/tradeoff here? I wonder if it would be best to directly expose both of those options..

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a handle on the difference/tradeoff here?

As far as the distinction between the two, my understanding is that it's analogous to GitHub's created_at and published_at fields respectively. The formers are the system derived values set to the true timestamp when the artifact was created, whereas the latters are a customizable field users can leverage to denote a "future" release, which I believe is only available when creating release via the API (for both Lab and Hub)

https://docs.gitlab.com/ee/api/releases/ says the default ordering is released_at so I guess we are saying ?sort=date corresponds to released_at (as opposed to created_at).

I could've sworn I'd left a note on this somewhere but can't find anything anywhere so 🤷‍♂️

In short, I've already found cases where their docs are just wrong as it relates to default ordering (see the Tags endpoint for example), so I don't personally put much stock into their claimed defaults.

The only case where I could potentially see this being relevant would be a scenario where someone is manually creating releases via the API and interleaving dates. That's probably a bit of an edge case, and one I'd prefer to wait to solve for only if someone actually asks, in part because we don't support the same for GitHub and because it introduces scenarios like if/how to account for future released_at/published_at dates.

We'd still have the flexibility down the road to add a new foo query param that could be used to toggle

Copy link
Member

Choose a reason for hiding this comment

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

OK. If we reckon the default in the docs may be misleading, should we explicitly pass an order_by param so we're clear which decision we're making them.
Trying to think about it from the point of view of compatibility with the GH badge, I'm now not actually sure what assumption the releases/latest endpoint makes on this.. I guess the reason I'm thinking we might expose both is because if we pick one then inevitably its the wrong one for someone. I'm thinking #6309 , #6236 , etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, we should at a minimum be explicit both in the params we surface on our routes and in what we pass upstream to make us as "future proof" as possible to allow for any changes/enhancements down the line without risking breaking changes.

Believe cc62b85 should cover this, though still want to punt on any considerations of dealing with future date values with the released_at ordering to our future selves 😆

errorMessages: {
404: 'project not found',
},
firstPageOnly: !isSemver,
})
}

static transform({ releases, isSemver, includePrereleases, displayName }) {
if (releases.length === 0) {
throw new NotFound({ prettyMessage: 'no releases found' })
}

const displayKey = displayName === 'tag' ? 'tag_name' : 'name'

if (!isSemver) {
return releases[0][displayKey]
}

return latest(
releases.map(t => t[displayKey]),
{ pre: includePrereleases }
)
}

async handle(
{ user, repo },
{
gitlab_url: baseUrl = 'https://gitlab.com',
include_prereleases: pre,
sort,
display_name: displayName,
}
) {
const isSemver = sort === 'semver'
const releases = await this.fetch({ user, repo, baseUrl, isSemver })
const version = this.constructor.transform({
releases,
isSemver,
includePrereleases: pre !== undefined,
displayName,
})
return renderVersionBadge({ version })
}
}
33 changes: 33 additions & 0 deletions services/gitlab/gitlab-release.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { isSemver, withRegex } from '../test-validators.js'
import { createServiceTester } from '../tester.js'
export const t = await createServiceTester()

const isGitLabDisplayVersion = withRegex(/^GitLab [1-9][0-9]*.[0-9]*/)

t.create('Release (latest by date)')
.get('/shields-ops-group/tag-test.json')
.expectBadge({ label: 'release', message: 'v2.0.0', 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.

Here's an example of a gitlab project which is nested in a group and has releases we could use for a service test: https://gitlab.com/gitlab-org/frontend/eslint-plugin/-/releases

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome! Excellent find and/or memory 😆

t.create('Release (latest by semver)')
.get('/shields-ops-group/tag-test.json?sort=semver')
.expectBadge({ label: 'release', message: 'v4.0.0', color: 'blue' })

t.create('Release (latest by semver pre-release)')
.get('/shields-ops-group/tag-test.json?sort=semver&include_prereleases')
.expectBadge({ label: 'release', message: 'v5.0.0-beta.1', color: 'orange' })

t.create('Release (release display name)')
.get('/gitlab-org/gitlab.json?display_name=release')
.expectBadge({ label: 'release', message: isGitLabDisplayVersion })

t.create('Release (custom instance')
.get('/GNOME/librsvg.json?gitlab_url=https://gitlab.gnome.org')
.expectBadge({ label: 'release', message: isSemver, color: 'blue' })

t.create('Release (project not found)')
.get('/fdroid/nonexistant.json')
.expectBadge({ label: 'release', message: 'project not found' })

t.create('Release (no tags)')
.get('/fdroid/fdroiddata.json')
.expectBadge({ label: 'release', message: 'no releases found' })