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

Migrate [Jira] services to new service model #2541

Merged
merged 5 commits into from
Dec 18, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
25 changes: 25 additions & 0 deletions services/jira/jira-base.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use strict'

const BaseJsonService = require('../base-json')
const serverSecrets = require('../../lib/server-secrets')

module.exports = class BaseJiraService extends BaseJsonService {
static get category() {
return 'issue-tracking'
}

async fetch({ url, options = {}, schema, errorMessages }) {
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
if (serverSecrets && serverSecrets.jira_username) {
options.auth = {
user: serverSecrets.jira_username,
pass: serverSecrets.jira_password,
}
}
return this._requestJson({
schema,
url,
options,
errorMessages,
})
}
}
134 changes: 63 additions & 71 deletions services/jira/jira-issue.service.js
Original file line number Diff line number Diff line change
@@ -1,100 +1,92 @@
'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 BaseJiraService = require('./jira-base')

module.exports = class JiraIssue extends LegacyService {
static get category() {
return 'issue-tracking'
const schema = Joi.object({
fields: Joi.object({
status: Joi.object({
name: Joi.string().required(),
statusCategory: Joi.object({
colorName: Joi.string().required(),
}),
}).required(),
}).required(),
}).required()

module.exports = class JiraIssue extends BaseJiraService {
static render({ issue, statusName, color }) {
return {
label: issue,
message: statusName,
color,
}
}

static get defaultBadgeData() {
return { color: 'lightgrey', label: 'jira' }
}

static get route() {
return {
base: 'jira/issue',
pattern: ':protocol(https?)/:host/:path?/:issueKey',
Copy link
Member

Choose a reason for hiding this comment

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

  • The regex in this pattern confused me for a moment. :protocol(http|https) seems clearer.
  • I think you need :path* to allow multiple path segments.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 on protocol

path is where I kept hitting a a mental wall with the pattern. I was trying to use nested non-capture groups but pattern was having none of it 😆 Let me see if I can clean that up a bit

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 ended up using a specific regex ((.+)) for the host param, which works and is more consistent with the legacy service info (which did not explicitly differentiate host and path)

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the change you made, though I think hostAndPath would better describe the field for the user (this is with #2496 in mind).

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I like that better too (and that's what was there before so +1 for consistency!)

Copy link
Member Author

@calebcartwright calebcartwright Dec 18, 2018

Choose a reason for hiding this comment

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

changed to hostAndPath in both the example pattern, route pattern, and param to handle for consistency. I elected not to include the + suffix but let me know if it is needed

(original comment) i'm probably being overly pedantic here, but just noticed the example pattern before was actually hostAndPath+ does the + suffix have any significance in the example patterns/is it worth including?

Copy link
Member

Choose a reason for hiding this comment

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

In a pattern, the + tells pathToRegexp to allow one or more segments instead of just one, zero or one, or zero or more. We aren’t doing anything using the pattern in the examples beyond parsing it to get the individual segments. (That’s forthcoming actually, in #2496)

}
}

static get examples() {
return [
{
title: 'JIRA issue',
pattern: ':protocol/:hostAndPath+/:issueKey',
pattern: ':protocol/:host/:issueKey',
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
namedParams: {
protocol: 'https',
hostAndPath: 'issues.apache.org/jira',
host: 'issues.apache.org/jira',
issueKey: 'KAFKA-2896',
},
staticPreview: {
label: 'kafka-2896',
message: 'Resolved',
staticPreview: this.render({
issue: 'KAFKA-2896',
statusName: 'Resolved',
color: 'green',
},
}),
keywords: ['jira', 'issue'],
},
]
}

static registerLegacyRouteHandler({ camp, cache }) {
camp.route(
/^\/jira\/issue\/(http(?:s)?)\/(.+)\/([^/]+)\.(svg|png|gif|jpg|json)$/,
cache((data, match, sendBadge, request) => {
const protocol = match[1] // eg, https
const host = match[2] // eg, issues.apache.org/jira
const issueKey = match[3] // eg, KAFKA-2896
const format = match[4]
async handle({ protocol, host, path, issueKey }) {
let url = `${protocol}://${host}`
if (path) {
url += `/${path}`
}

const options = {
method: 'GET',
json: true,
uri: `${protocol}://${host}/rest/api/2/issue/${encodeURIComponent(
issueKey
)}`,
}
if (serverSecrets && serverSecrets.jira_username) {
options.auth = {
user: serverSecrets.jira_username,
pass: serverSecrets.jira_password,
}
}
// Atlassian Documentation: https://developer.atlassian.com/cloud/jira/platform/rest/v2/#api-api-2-issue-issueIdOrKey-get
url += `/rest/api/2/issue/${encodeURIComponent(issueKey)}`

// map JIRA color names to closest shields color schemes
const colorMap = {
'medium-gray': 'lightgrey',
green: 'green',
yellow: 'yellow',
brown: 'orange',
'warm-red': 'red',
'blue-gray': 'blue',
}
const json = await this.fetch({
url,
schema,
options: {},
errorMessages: {
404: 'issue not found',
},
})
const issueStatus = json.fields.status
const statusName = issueStatus.name
let color = 'lightgrey'
if (issueStatus.statusCategory) {
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
// map JIRA color names to closest shields color schemes
const colorMap = {
'medium-gray': 'lightgrey',
green: 'green',
yellow: 'yellow',
brown: 'orange',
'warm-red': 'red',
'blue-gray': 'blue',
}
color = colorMap[issueStatus.statusCategory.colorName]
}

const badgeData = getBadgeData(issueKey, data)
request(options, (err, res, json) => {
if (err !== null) {
badgeData.text[1] = 'inaccessible'
sendBadge(format, badgeData)
return
}
try {
const jiraIssue = json
if (jiraIssue.fields && jiraIssue.fields.status) {
if (jiraIssue.fields.status.name) {
badgeData.text[1] = jiraIssue.fields.status.name // e.g. "In Development"
}
if (jiraIssue.fields.status.statusCategory) {
badgeData.colorscheme =
colorMap[jiraIssue.fields.status.statusCategory.colorName] ||
'lightgrey'
}
} else {
badgeData.text[1] = 'invalid'
}
sendBadge(format, badgeData)
} catch (e) {
badgeData.text[1] = 'invalid'
sendBadge(format, badgeData)
}
})
})
)
return this.constructor.render({ issue: issueKey, statusName, color })
}
}
71 changes: 71 additions & 0 deletions services/jira/jira-issue.tester.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
'use strict'

const t = (module.exports = require('../create-service-tester')())

t.create('live: unknown issue')
.get('/https/issues.apache.org/jira/notArealIssue-000.json')
.expectJSON({ name: 'jira', value: 'issue not found' })

t.create('live: known issue')
.get('/https/issues.apache.org/jira/kafka-2896.json')
.expectJSON({ name: 'kafka-2896', value: 'Resolved' })

t.create('http endpoint')
.get('/http/issues.apache.org/jira/foo-123.json')
.intercept(nock =>
nock('http://issues.apache.org/jira/rest/api/2/issue')
.get(`/${encodeURIComponent('foo-123')}`)
.reply(200, {
fields: {
status: {
name: 'pending',
},
},
})
)
.expectJSON({ name: 'foo-123', value: 'pending' })

t.create('endpoint with port and path')
.get('/https/issues.apache.org:8000/jira/bar-345.json')
.intercept(nock =>
nock('https://issues.apache.org:8000/jira/rest/api/2/issue')
.get(`/${encodeURIComponent('bar-345')}`)
.reply(200, {
fields: {
status: {
name: 'done',
},
},
})
)
.expectJSON({ name: 'bar-345', value: 'done' })

t.create('endpoint with port and no path')
calebcartwright marked this conversation as resolved.
Show resolved Hide resolved
.get('/https/issues.apache.org:8080/abc-123.json')
.intercept(nock =>
nock('https://issues.apache.org:8080/rest/api/2/issue')
.get(`/${encodeURIComponent('abc-123')}`)
.reply(200, {
fields: {
status: {
name: 'under review',
},
},
})
)
.expectJSON({ name: 'abc-123', value: 'under review' })

t.create('endpoint with no port nor path')
.get('/https/issues.apache.org/test-001.json')
.intercept(nock =>
nock('https://issues.apache.org/rest/api/2/issue')
.get(`/${encodeURIComponent('test-001')}`)
.reply(200, {
fields: {
status: {
name: 'in progress',
},
},
})
)
.expectJSON({ name: 'test-001', value: 'in progress' })
Loading