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 [snapcraft] version badge #9976

Merged
merged 17 commits into from
Mar 10, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
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
91 changes: 91 additions & 0 deletions services/snapcraft/snapcraft-version.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import Joi from 'joi'
import { BaseJsonService, NotFound, pathParams, queryParam } from '../index.js'
import { renderVersionBadge } from '../version.js'

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

const versionSchema = Joi.object({
'channel-map': Joi.array()
.items(
Joi.object({
channel: Joi.object({
architecture: Joi.string().required(),
risk: Joi.string().required(),
track: Joi.string().required(),
}),
version: Joi.string().required(),
}).required(),
)
.min(1)
.required(),
}).required()

export default class SnapcraftVersion extends BaseJsonService {
static category = 'version'

static route = {
base: 'snapcraft/v',
pattern: ':package/:track/:risk',
queryParamSchema,
}

static defaultBadgeData = { label: 'snapcraft' }

static openApi = {
'/snapcraft/v/{package}/{track}/{risk}': {
get: {
summary: 'Snapcraft version',
parameters: [
...pathParams(
{ name: 'package', example: 'chromium' },
{ name: 'track', example: 'latest' },
{ name: 'risk', example: 'stable' },
),
queryParam({
name: 'arch',
example: 'amd64',
description:
'Architecture, When not specified, this will default to `amd64`.',
}),
],
},
},
}

async handle({ package: packageName, track, risk }, { arch = 'amd64' }) {
const parsedData = await this._requestJson({
schema: versionSchema,
options: {
headers: { 'Snap-Device-Series': 16 },
},
url: `https://api.snapcraft.io/v2/snaps/info/${packageName}`,
httpErrors: {
404: 'package not found',
},
})

const channelMap = parsedData['channel-map']
let filteredChannelMap = channelMap.filter(
({ channel }) => channel.architecture === arch,
)
if (filteredChannelMap.length === 0) {
throw new NotFound({ prettyMessage: 'arch not found' })
}
filteredChannelMap = channelMap.filter(
({ channel }) => channel.track === track,
)
if (filteredChannelMap.length === 0) {
throw new NotFound({ prettyMessage: 'track not found' })
}
filteredChannelMap = channelMap.filter(
({ channel }) => channel.risk === risk,
)
if (filteredChannelMap.length === 0) {
throw new NotFound({ prettyMessage: 'risk not found' })
}
Copy link
Member

@chris48s chris48s Mar 3, 2024

Choose a reason for hiding this comment

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

This code looks wrong to me. These filters should be combined with AND logic, but we're filtering channelMap, assigning the result to filteredChannelMapand then essentially throwing away that result and overwriting filteredChannelMap. We're only really applying the last filter.

I think you really want to be doing

filteredChannelMap = filteredChannelMap.filter(...

for the second and third pass.

What would really help here would be if we split this filtering logic out into a transform() function and write a few unit tests (in a spec.js) to make sure this is working as expected.

You might look at something like

services/docker/docker-version.spec.js
or
services/crates/crates-base.spec.js

as a point of inspiration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad, i copy-pasted without noticing i forgot to change to filteredChannelMap

Fixed filter logic to AND with b54f1e6
Moved the filter logic into transform with 75f8ac7
And most impotent, added testing with 29dd792
This test file is really called for.


return renderVersionBadge({ version: filteredChannelMap[0].version })
}
}
45 changes: 45 additions & 0 deletions services/snapcraft/snapcraft-version.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { isSemver } from '../test-validators.js'
import { createServiceTester } from '../tester.js'
export const t = await createServiceTester()

t.create('Snapcraft Version for redis')
.get('/redis/latest/stable.json')
.expectBadge({
label: 'snapcraft',
message: isSemver,
})

t.create('Snapcraft Version for redis (query param arch=arm64)')
.get('/redis/latest/stable.json?arch=arm64')
.expectBadge({
label: 'snapcraft',
message: isSemver,
})

t.create('Snapcraft Version for redis (invalid package)')
.get('/this_package_doesnt_exist/fake/fake.json')
.expectBadge({
label: 'snapcraft',
message: 'package not found',
})

t.create('Snapcraft Version for redis (invalid track)')
.get('/redis/notfound/stable.json')
.expectBadge({
label: 'snapcraft',
message: 'track not found',
})

t.create('Snapcraft Version for redis (invalid risk)')
.get('/redis/latest/notfound.json')
.expectBadge({
label: 'snapcraft',
message: 'risk not found',
})

t.create('Snapcraft Version for redis (invalid arch)')
.get('/redis/latest/stable.json?arch=fake')
.expectBadge({
label: 'snapcraft',
message: 'arch not found',
})
Loading