Skip to content

Commit

Permalink
Drop gif + png, and redirect png to raster.shields.io (badges#3644)
Browse files Browse the repository at this point in the history
1. Remove rasterization support from the server. This responsibility is delegated to a raster server which proxies the SVG badges and renders them.
2. When a raster server URL is configured, 301 redirect all .png badges to the identical URL on the raster server.
    `https://img.shields.io/npm/v/express.png?style=flat-square` ↪️`https://raster.shields.io/npm/v/express.png?style=flat-square`
3. For configured redirects, redirect to the canonical URL on the raster server.
    `https://img.shields.io/vso/build/totodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/2.png?style=flat-square`
    ↪️`https://img.shields.io/azure-devops/build/totodem/8cf3ec0e-d0c2-4fcd-8206-ad204f254a96/2.png?style=flat-square`
4. Redirect the "legacy badge old version" to the appropriate URL on the raster server.
5. When no raster server is configured (e.g. PRs), render an SVG containing **404 | raster badges not available** for all `.png` badges. (Note that the raster server can be self-hosted; however, this is deferred to a later PR.)
5. Drop support for jpg and gif which are very infrequently used (see badges#3112). Render an SVG containing **410 | jpg no longer available**.
7. ~~Remove raster dependencies.~~ Remove the raster cache (which is only used in the CLI, and therefore pointless).
8. Move the LRUCache code out of the npm package.
8. A wee bit of refactoring in `server.js`.

Ref badges#3112
Close badges#3631
  • Loading branch information
paulmelnikow authored Jul 6, 2019
1 parent c6ef885 commit 66c7f13
Show file tree
Hide file tree
Showing 21 changed files with 166 additions and 154 deletions.
2 changes: 0 additions & 2 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
FROM node:8-alpine

RUN apk add --no-cache gettext imagemagick librsvg git

RUN mkdir -p /usr/src/app
RUN mkdir /usr/src/app/private
WORKDIR /usr/src/app
Expand Down
2 changes: 2 additions & 0 deletions config/custom-environment-variables.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public:

redirectUri: 'REDIRECT_URI'

rasterUrl: 'RASTER_URL'

cors:
allowedOrigin:
__name: 'ALLOWED_ORIGIN'
Expand Down
2 changes: 2 additions & 0 deletions config/shields-io-production.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ public:

redirectUrl: 'https://shields.io/'

rasterUrl: 'https://raster.shields.io'

private:
# These are not really private; they should be moved to `public`.
shields_ips: ['192.99.59.72', '51.254.114.150', '149.56.96.133']
2 changes: 2 additions & 0 deletions config/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@ public:

redirectUrl: 'http://badge-server.example.com'

rasterUrl: 'http://raster.example.com'

handleInternalErrors: false
11 changes: 11 additions & 0 deletions core/badge-urls/make-badge-url.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'

const { URL } = require('url')
const queryString = require('query-string')
const pathToRegexp = require('path-to-regexp')

Expand Down Expand Up @@ -134,11 +135,21 @@ function dynamicBadgeUrl({
return `${baseUrl}/badge/dynamic/${datatype}.${format}?${outQueryString}`
}

function rasterRedirectUrl({ rasterUrl }, badgeUrl) {
// Ensure we're always using the `rasterUrl` by using just the path from
// the request URL.
const { pathname, search } = new URL(badgeUrl, 'https://bogus.test')
const result = new URL(pathname, rasterUrl)
result.search = search
return result
}

module.exports = {
badgeUrlFromPath,
badgeUrlFromPattern,
encodeField,
staticBadgeUrl,
queryStringStaticBadgeUrl,
dynamicBadgeUrl,
rasterRedirectUrl,
}
2 changes: 1 addition & 1 deletion core/base-service/base.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ describe('BaseService', function() {
})

describe('ScoutCamp integration', function() {
const expectedRouteRegex = /^\/foo\/([^/]+?)\.(svg|png|gif|jpg|json)$/
const expectedRouteRegex = /^\/foo\/([^/]+?)\.(svg|json)$/

let mockCamp
let mockHandleRequest
Expand Down
2 changes: 1 addition & 1 deletion core/base-service/legacy-request-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
const domain = require('domain')
const request = require('request')
const queryString = require('query-string')
const LruCache = require('../../gh-badges/lib/lru-cache')
const makeBadge = require('../../gh-badges/lib/make-badge')
const log = require('../server/log')
const { setCacheHeaders } = require('./cache-headers')
Expand All @@ -14,6 +13,7 @@ const {
ShieldsRuntimeError,
} = require('./errors')
const { makeSend } = require('./legacy-result-sender')
const LruCache = require('./lru-cache')
const coalesceBadge = require('./coalesce-badge')

// We avoid calling the vendor's server for computation of the information in a
Expand Down
26 changes: 1 addition & 25 deletions core/base-service/legacy-result-sender.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
'use strict'

const fs = require('fs')
const path = require('path')
const stream = require('stream')
const svg2img = require('../../gh-badges/lib/svg-to-img')
const log = require('../server/log')

const internalError = fs.readFileSync(
path.resolve(__dirname, '..', 'server', 'error-pages', '500.html'),
'utf-8'
)

function streamFromString(str) {
const newStream = new stream.Readable()
Expand All @@ -25,21 +16,6 @@ function sendSVG(res, askres, end) {
end(null, { template: streamFromString(res) })
}

function sendOther(format, res, askres, end) {
askres.setHeader('Content-Type', `image/${format}`)
svg2img(res, format)
// This interacts with callback code and can't use async/await.
// eslint-disable-next-line promise/prefer-await-to-then
.then(data => {
end(null, { template: streamFromString(data) })
})
.catch(err => {
// This emits status code 200, though 500 would be preferable.
log.error('svg2img error', err)
end(internalError)
})
}

function sendJSON(res, askres, end) {
askres.setHeader('Content-Type', 'application/json')
askres.setHeader('Access-Control-Allow-Origin', '*')
Expand All @@ -52,7 +28,7 @@ function makeSend(format, askres, end) {
} else if (format === 'json') {
return res => sendJSON(res, askres, end)
} else {
return res => sendOther(format, res, askres, end)
throw Error(`Unrecognized format: ${format}`)
}
}

Expand Down
File renamed without changes.
File renamed without changes.
11 changes: 8 additions & 3 deletions core/base-service/redirector.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,11 @@ module.exports = function redirector(attrs) {
return route
}

static register({ camp, requestCounter }) {
const { regex, captureNames } = prepareRoute(this.route)
static register({ camp, requestCounter }, { rasterUrl }) {
const { regex, captureNames } = prepareRoute({
...this.route,
withPng: Boolean(rasterUrl),
})

const serviceRequestCounter = this._createServiceRequestCounter({
requestCounter,
Expand Down Expand Up @@ -104,7 +107,9 @@ module.exports = function redirector(attrs) {

// The final capture group is the extension.
const format = match.slice(-1)[0]
const redirectUrl = `${targetPath}.${format}${urlSuffix}`
const redirectUrl = `${
format === 'png' ? rasterUrl : ''
}${targetPath}.${format}${urlSuffix}`
trace.logTrace('outbound', emojic.shield, 'Redirect URL', redirectUrl)

ask.res.statusCode = 301
Expand Down
11 changes: 8 additions & 3 deletions core/base-service/redirector.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ describe('Redirector', function() {
transformPath,
dateAdded,
})
ServiceClass.register({ camp }, {})
ServiceClass.register(
{ camp },
{ rasterUrl: 'http://raster.example.test' }
)
})

it('should redirect as configured', async function() {
Expand All @@ -90,7 +93,7 @@ describe('Redirector', function() {
expect(headers.location).to.equal('/new/service/hello-world.svg')
})

it('should preserve the extension', async function() {
it('should redirect raster extensions to the canonical path as configured', async function() {
const { statusCode, headers } = await got(
`${baseUrl}/very/old/service/hello-world.png`,
{
Expand All @@ -99,7 +102,9 @@ describe('Redirector', function() {
)

expect(statusCode).to.equal(301)
expect(headers.location).to.equal('/new/service/hello-world.png')
expect(headers.location).to.equal(
'http://raster.example.test/new/service/hello-world.png'
)
})

it('should forward the query params', async function() {
Expand Down
14 changes: 6 additions & 8 deletions core/base-service/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,16 @@ function assertValidRoute(route, message = undefined) {
Joi.assert(route, isValidRoute, message)
}

function prepareRoute({ base, pattern, format, capture }) {
function prepareRoute({ base, pattern, format, capture, withPng }) {
const extensionRegex = ['svg', 'json']
.concat(withPng ? ['png'] : [])
.join('|')
let regex, captureNames
if (pattern === undefined) {
regex = new RegExp(
`^${makeFullUrl(base, format)}\\.(svg|png|gif|jpg|json)$`
)
regex = new RegExp(`^${makeFullUrl(base, format)}\\.(${extensionRegex})$`)
captureNames = capture || []
} else {
const fullPattern = `${makeFullUrl(
base,
pattern
)}.:ext(svg|png|gif|jpg|json)`
const fullPattern = `${makeFullUrl(base, pattern)}.:ext(${extensionRegex})`
const keys = []
regex = pathToRegexp(fullPattern, keys, {
strict: true,
Expand Down
9 changes: 0 additions & 9 deletions core/base-service/route.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@ describe('Route helpers', function() {
test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({ namedParamA: 'bar.bar.bar' })
})
Expand Down Expand Up @@ -64,9 +61,6 @@ describe('Route helpers', function() {
test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({ namedParamA: 'bar.bar.bar' })
})
Expand All @@ -83,9 +77,6 @@ describe('Route helpers', function() {
test(namedParams, () => {
forCases([
given('/foo/bar.bar.bar.svg'),
given('/foo/bar.bar.bar.png'),
given('/foo/bar.bar.bar.gif'),
given('/foo/bar.bar.bar.jpg'),
given('/foo/bar.bar.bar.json'),
]).expect({})
})
Expand Down
Loading

0 comments on commit 66c7f13

Please sign in to comment.