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

refactor [JenkinsBuild] #3302

Merged
merged 10 commits into from
Apr 19, 2019
30 changes: 30 additions & 0 deletions services/jenkins/jenkins-base.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
'use strict'

const { BaseJsonService } = require('..')
const serverSecrets = require('../../lib/server-secrets')

module.exports = class JenkinsBase extends BaseJsonService {
async fetch({
url,
schema,
qs,
errorMessages = { 404: 'instance or job not found' },
disableStrictSSL,
}) {
const options = { qs, strictSSL: disableStrictSSL === undefined }
Copy link
Member Author

Choose a reason for hiding this comment

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

See below comment in jenkins-common.js for more info on param name


if (serverSecrets.jenkins_user) {
options.auth = {
user: serverSecrets.jenkins_user,
pass: serverSecrets.jenkins_pass,
}
}

return this._requestJson({
url,
options,
schema,
errorMessages,
})
}
}
16 changes: 16 additions & 0 deletions services/jenkins/jenkins-build-redirect.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict'

const { redirector } = require('..')

module.exports = [
redirector({
category: 'build',
route: {
base: 'jenkins-ci',
pattern: 's/:protocol(http|https)/:host+/:job+',
},
transformPath: ({ protocol, host, job }) =>
`/jenkins/s/${protocol}/${host}/${job}`,
dateAdded: new Date('2019-04-14'),
}),
]
19 changes: 19 additions & 0 deletions services/jenkins/jenkins-build-redirect.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict'

const { ServiceTester } = require('../tester')

const t = (module.exports = new ServiceTester({
id: 'JenkinsBuildRedirect',
title: 'JenkinsBuildRedirect',
pathPrefix: '/jenkins-ci/s',
}))

t.create('jenkins ci')
.get('/https/updates.jenkins-ci.org/job/foo.svg', {
followRedirect: false,
})
.expectStatus(301)
.expectHeader(
'Location',
'/jenkins/s/https/updates.jenkins-ci.org/job/foo.svg'
)
140 changes: 64 additions & 76 deletions services/jenkins/jenkins-build.service.js
Original file line number Diff line number Diff line change
@@ -1,107 +1,95 @@
'use strict'

const LegacyService = require('../legacy-service')
const { makeBadgeData: getBadgeData } = require('../../lib/badge-data')
const serverSecrets = require('../../lib/server-secrets')
const Joi = require('joi')
const { renderBuildStatusBadge } = require('../build-status')
const JenkinsBase = require('./jenkins-base')
const {
buildTreeParamQueryString,
buildUrl,
queryParamSchema,
} = require('./jenkins-common')

// This legacy service should be rewritten to use e.g. BaseJsonService.
//
// Tips for rewriting:
// https://github.com/badges/shields/blob/master/doc/rewriting-services.md
//
// Do not base new services on this code.
module.exports = class JenkinsBuild extends LegacyService {
// https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/model/BallColor.java#L56
const colorStatusMap = {
red: 'failing',
red_anime: 'building',
yellow: 'unstable',
yellow_anime: 'building',
blue: 'passing',
blue_anime: 'building',
green: 'passing',
green_anime: 'building',
grey: 'not built',
grey_anime: 'building',
disabled: 'not built',
disabled_anime: 'building',
aborted: 'not built',
aborted_anime: 'building',
notbuilt: 'not built',
notbuilt_anime: 'building',
}

const schema = Joi.object({
color: Joi.allow(...Object.keys(colorStatusMap)).required(),
}).required()

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

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

static get route() {
return {
base: 'jenkins/s',
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 don't have any PR #'s handy, but we seem to be trending away from single letter aliases to more explicit route param names.

I'd like to do the same thing here (build or status instead of s). Any thoughts/objections?

Copy link
Member

Choose a reason for hiding this comment

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

Happy if you want to change it, but we'll want to maintain an alias/redirect for /s for existing users

We talked about formalising some of this stuff in docs in #3144 (comment) and then.. haven't got round to doing it yet. There's several things in that discussion that I want to get done at some point but haven't found the time. Having this stuff documented would make it easier when we're thinking

wait, what is our convention for a "build status" URL?

🙂

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'll leave it in place for now. I plan on using some of the common bits in here to refactor the Jenkins test service so I'll consider the alias redirecting in that one

pattern: ':scheme(http|https)?/:host/:job*',
pattern: ':protocol(http|https)/:host/:job+',
queryParamSchema,
}
}

static get examples() {
return [
{
title: 'Jenkins',
pattern: ':scheme/:host/:job',
namedParams: {
scheme: 'https',
protocol: 'https',
host: 'jenkins.qa.ubuntu.com',
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 am also intentionally not including the new disableStrictSSL query param here as I do not believe this is something we want to advertise; it's a work around for a handful of exceptional circumstances only.

job:
'view/Precise/view/All%20Precise/job/precise-desktop-amd64_default',
},
staticPreview: {
label: 'build',
message: 'passing',
color: 'brightgreen',
},
staticPreview: renderBuildStatusBadge({ status: 'passing' }),
},
]
}

static registerLegacyRouteHandler({ camp, cache }) {
camp.route(
/^\/jenkins(?:-ci)?\/s\/(http(?:s)?)\/([^/]+)\/(.+)\.(svg|png|gif|jpg|json)$/,
cache((data, match, sendBadge, request) => {
const scheme = match[1] // http(s)
const host = match[2] // example.org:8080
const job = match[3] // folder/job
const format = match[4]
const options = {
json: true,
uri: `${scheme}://${host}/job/${job}/api/json?tree=color`,
}
if (job.indexOf('/') > -1) {
options.uri = `${scheme}://${host}/${job}/api/json?tree=color`
}
static render({ status }) {
if (status === 'unstable') {
return {
message: status,
color: 'yellow',
}
}

if (serverSecrets.jenkins_user) {
options.auth = {
user: serverSecrets.jenkins_user,
pass: serverSecrets.jenkins_pass,
}
}
return renderBuildStatusBadge({ status })
}

const badgeData = getBadgeData('build', data)
request(options, (err, res, json) => {
if (err !== null) {
badgeData.text[1] = 'inaccessible'
sendBadge(format, badgeData)
return
}
transform({ json }) {
return { status: colorStatusMap[json.color] }
}

try {
if (json.color === 'blue' || json.color === 'green') {
badgeData.colorscheme = 'brightgreen'
badgeData.text[1] = 'passing'
} else if (json.color === 'red') {
badgeData.colorscheme = 'red'
badgeData.text[1] = 'failing'
} else if (json.color === 'yellow') {
badgeData.colorscheme = 'yellow'
badgeData.text[1] = 'unstable'
} else if (
json.color === 'grey' ||
json.color === 'disabled' ||
json.color === 'aborted' ||
json.color === 'notbuilt'
) {
badgeData.colorscheme = 'lightgrey'
badgeData.text[1] = 'not built'
} else {
badgeData.colorscheme = 'lightgrey'
badgeData.text[1] = 'building'
}
sendBadge(format, badgeData)
} catch (e) {
badgeData.text[1] = 'invalid'
sendBadge(format, badgeData)
}
})
})
)
async handle({ protocol, host, job }, { disableStrictSSL }) {
const json = await this.fetch({
url: buildUrl({ protocol, host, job, lastBuild: false }),
schema,
qs: buildTreeParamQueryString('color'),
disableStrictSSL,
})
const { status } = this.transform({ json })
return this.constructor.render({ status })
}
}
57 changes: 57 additions & 0 deletions services/jenkins/jenkins-build.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict'

const { test, forCases, given } = require('sazerac')
const { renderBuildStatusBadge } = require('../build-status')
const JenkinsBuild = require('./jenkins-build.service')

describe('JenkinsBuild', function() {
test(JenkinsBuild.prototype.transform, () => {
forCases([
given({ json: { color: 'red_anime' } }),
given({ json: { color: 'yellow_anime' } }),
given({ json: { color: 'blue_anime' } }),
given({ json: { color: 'green_anime' } }),
given({ json: { color: 'grey_anime' } }),
given({ json: { color: 'disabled_anime' } }),
given({ json: { color: 'aborted_anime' } }),
given({ json: { color: 'notbuilt_anime' } }),
]).expect({
status: 'building',
})
forCases([
given({ json: { color: 'grey' } }),
given({ json: { color: 'disabled' } }),
given({ json: { color: 'aborted' } }),
given({ json: { color: 'notbuilt' } }),
]).expect({
status: 'not built',
})
forCases([
given({ json: { color: 'blue' } }),
given({ json: { color: 'green' } }),
]).expect({
status: 'passing',
})
given({ json: { color: 'red' } }).expect({ status: 'failing' })
given({ json: { color: 'yellow' } }).expect({ status: 'unstable' })
})

test(JenkinsBuild.render, () => {
given({ status: 'unstable' }).expect({
message: 'unstable',
color: 'yellow',
})
given({ status: 'passing' }).expect(
renderBuildStatusBadge({ status: 'passing' })
)
given({ status: 'failing' }).expect(
renderBuildStatusBadge({ status: 'failing' })
)
given({ status: 'building' }).expect(
renderBuildStatusBadge({ status: 'building' })
)
given({ status: 'not built' }).expect(
renderBuildStatusBadge({ status: 'not built' })
)
})
})
53 changes: 53 additions & 0 deletions services/jenkins/jenkins-build.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict'

const Joi = require('joi')
const sinon = require('sinon')
const serverSecrets = require('../../lib/server-secrets')
const { isBuildStatus } = require('../build-status')
const t = (module.exports = require('../tester').createServiceTester())

const isJenkinsBuildStatus = Joi.alternatives(
isBuildStatus,
Joi.string().allow('unstable')
)

t.create('build job not found')
.get('/https/updates.jenkins-ci.org/job/does-not-exist.json')
.expectBadge({ label: 'build', message: 'instance or job not found' })

t.create('build found (view)')
.get(
'/https/jenkins.qa.ubuntu.com/view/Precise/view/All%20Precise/job/precise-desktop-amd64_default.json'
)
.expectBadge({ label: 'build', message: isJenkinsBuildStatus })

t.create('build found (job)')
.get('/https/jenkins.ubuntu.com/server/curtin-vmtest-daily-x.json')
.expectBadge({ label: 'build', message: isJenkinsBuildStatus })

const user = 'admin'
const pass = 'password'

function mockCreds() {
serverSecrets['jenkins_user'] = undefined
serverSecrets['jenkins_pass'] = undefined
sinon.stub(serverSecrets, 'jenkins_user').value(user)
sinon.stub(serverSecrets, 'jenkins_pass').value(pass)
}

t.create('with mock credentials')
.before(mockCreds)
.get('/https/jenkins.ubuntu.com/server/job/curtin-vmtest-daily-x.json')
.intercept(nock =>
nock('https://jenkins.ubuntu.com/server/job/curtin-vmtest-daily-x')
.get(`/api/json?tree=color`)
// This ensures that the expected credentials from serverSecrets are actually being sent with the HTTP request.
// Without this the request wouldn't match and the test would fail.
.basicAuth({
user,
pass,
})
.reply(200, { color: 'blue' })
)
.finally(sinon.restore)
.expectBadge({ label: 'build', message: 'passing' })
20 changes: 20 additions & 0 deletions services/jenkins/jenkins-common.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict'

const Joi = require('joi')

const queryParamSchema = Joi.object({
disableStrictSSL: Joi.equal(''),
Copy link
Member Author

@calebcartwright calebcartwright Apr 18, 2019

Choose a reason for hiding this comment

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

Although I usually try to refrain from using negated variable names, naming this with the disable prefix seemed to be the best approach given the default behavior we want here and the way boolean query params are handled (empty string when set, undefined when not set).

With this approach, a user will have to explicitly include the disableStrictSSL query param:
https://shields-staging-pr-3302.herokuapp.com/jenkins/s/https/jenkins-oss.wixpress.com/job/detox-android-56-master.svg?disableStrictSSL

As the default behavior (no query param provided) will still enforce strict ssl, which is why the below badge will fail (there's a cert chain issue with the target jenkins instance)
https://shields-staging-pr-3302.herokuapp.com/jenkins/s/https/jenkins-oss.wixpress.com/job/detox-android-56-master.svg

Definitely open to feedback/suggestions/etc. if anyone has any alternative ideas!

}).required()

const buildUrl = ({ protocol, host, job, lastBuild = true, plugin }) => {
const jobPrefix = job.indexOf('/') > -1 ? '' : 'job/'
return `${protocol}://${host}/${jobPrefix}${job}/${
lastBuild ? 'lastBuild/' : ''
}${plugin ? `${plugin}/` : ''}api/json`
}
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'm setting this function up to be re-used in all the other Jenkins services (Tests, etc.) which is why it contains things not explicitly used for the Build service badge


module.exports = {
queryParamSchema,
buildTreeParamQueryString: tree => ({ tree }),
buildUrl,
}