Move [StaticBadge] to own service & add test; also affects [gitter] #2284

paulmelnikow · 2018-11-07T02:08:31Z

This picks up @RedSparr0w's work in #1802.

The handler for the static badge is moved into its own service with a synchronous handler. Avoiding an async call may make the static badges slightly faster, though it may be worth profiling this if it turns out we want asynchronous static badges in the future. If it doesn't make a performance difference we could make this handler async like the others.
Most of the custom static-badge logic is in a BaseStaticBadge base class.
Rewrite the static Gitter badge to use BaseStaticBadge.
A bit of minor cleanup in related functions.

Co-Authored-By: paulmelnikow <email@paulmelnikow.com>

# Conflicts: # server.js # services/base.js

shields-ci · 2018-11-07T02:09:52Z

	Warnings
⚠️	This PR modified helper functions in `lib/` but not accompanying tests. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `gitter` but not its test code. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

paulmelnikow · 2018-11-07T02:09:38Z

lib/request-handler.js

-    maxAge = parseInt(handlerOptions.cacheLength)
+    // If we've set a more specific cache length for this badge (or category),
+    // use that instead of env.BADGE_MAX_AGE_SECONDS.
+    maxAge = handlerOptions.cacheLength


Technically unrelated, as in the current implementation the static badge does not call this function.

There's no need for parseInt since we're passing numbers, not strings.

paulmelnikow · 2018-11-07T02:11:26Z

services/base-static.js

+const { makeSend } = require('../lib/result-sender')
+const BaseService = require('./base')
+
+const serverStartTime = new Date(new Date().toGMTString())


There's no point in passing this down from server.js.

paulmelnikow · 2018-11-07T02:13:08Z

services/base.js

@@ -214,6 +219,52 @@ class BaseService {
    return result
  }

+  _handleError(error) {


Extract into a method so BaseStaticService can invoke it from overridden register().

paulmelnikow · 2018-11-07T02:13:37Z

services/base.js

-        overrideLabel || serviceLabel || defaultLabel || this.category,
-        serviceMessage || 'n/a',
+        coalesce(overrideLabel, serviceLabel, defaultLabel, this.category),
+        coalesce(serviceMessage, 'n/a'),


This is covered by the static badge tests @RedSparr0w wrote.

paulmelnikow · 2018-11-07T02:15:23Z

services/base.js

@@ -318,28 +327,26 @@ class BaseService {
  }

  static register({ camp, handleRequest, githubApiProvider }, serviceConfig) {
-    const ServiceClass = this // In a static context, "this" is the class.


Some references use this and some use ServiceClass. Don't really care which we use, though consistent = better.

paulmelnikow · 2018-11-07T02:17:20Z

services/base-static.js

+        ask.res.statusCode = 304
+        ask.res.end()
+        return
+      }


Currently this behavior is not tested.

paulmelnikow · 2018-11-07T02:17:25Z

services/base-static.js

+
+      const cacheDuration = 3600 * 24 * 1 // 1 day.
+      ask.res.setHeader('Cache-Control', `max-age=${cacheDuration}`)
+      ask.res.setHeader('Last-Modified', serverStartTime.toGMTString())


Currently this behavior is not tested.

chris48s · 2018-11-16T22:14:04Z

I just realised this breaks the "no label" case e.g:

- https://img.shields.io/badge/all%20one%20color-red.svg
- https://shields-staging-pr-2284.herokuapp.com/badge/all%20one%20color-red.svg

paulmelnikow · 2018-11-16T22:22:25Z

Good catch!

shields-deployment · 2018-11-17T00:21:50Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

This regression from #2284 was causing `{ message: 22 }` to render as `’n/a’`, as in this test run: https://circleci.com/gh/badges/shields/23680

Now that the static badge has been moved in #2284, next in line for cleaning out `server.js` is this “static badge, old format.” I imagine this route is _very, very old_. (Actually I wouldn’t be surprised if it’s not used at all. I’d be cuious to see some stats on that endpoint.) In the case of URLs which have permanently changed, an approach I’d like to try is issuing a 301 Redirect. The benefit is that if a user pastes the URL into the address bar while they are previewing or editing it, the browser will replace the address with the corrected URL when it loads. I figure this will cause some people to update their URLs with no effort, simply because they previewed the badge in their browser, and others to change over, if they notice it. We incur a slight cost, which is a second request. However many browsers cache the 301’s indefinitely, and we can set an effectively infinite cache duration so the CDN and most other downstream caches will keep them a long time. And handling the redirect is extremely cheap. This is a nice way to preserve backward compatibility of old routes without having to complicate the new route, such as in the case of vso -> azure-devops. For maintenance purposes, the route that redirects can effectively be treated separately. It’s also a nice, gentle, and confidence-inspiring way to signal that users should update their URLs.

This regression from #2284 was causing `{ message: 22 }` to render as `’n/a’`, as in this test run: https://circleci.com/gh/badges/shields/23680

Now that the static badge has been moved in #2284, next in line for cleaning out `server.js` is this “static badge, old format.” I imagine this route is _very, very old_. (I wouldn’t be surprised if it’s not used at all. I’d be curious to see some stats on that endpoint. If it's not regularly getting requests I could see dropping it.) In the case of URLs which have permanently changed, an approach I’d like to try is issuing a 301 Redirect. The benefit is that if a user pastes the URL into the address bar while they are previewing or editing it, the browser will replace the address with the corrected URL when it loads. I figure this will cause some people to update their URLs with no effort, simply because they previewed the badge in their browser, and others to change over, if they notice it. We incur a slight cost, which is a second request. However many browsers cache the 301’s indefinitely, and we can set an effectively infinite cache duration so the CDN and most other downstream caches will keep them a long time. And handling the redirect is extremely cheap. This is a nice way to preserve backward compatibility of old routes without having to complicate the new route, such as in the case of vso -> azure-devops. For maintenance purposes, the route that redirects can effectively be treated separately. It’s also a nice, gentle, and confidence-inspiring way to signal that users should update their URLs. We could generalize this code, though I think this is a good place to start. This route is tricky because it needs to be loaded last, complicating a reusable solution.

RedSparr0w and others added 21 commits July 24, 2018 09:52

move static badge to own service & add test

b2d9016

[eslint]

c1c2ce0

add category

612ac05

fix test

5abac51

add coalesce function, support empty label & message

b85a380

Co-Authored-By: paulmelnikow <email@paulmelnikow.com>

update test

3b3bc36

Merge branch 'master' into static-service

3613b58

update category

64db5b1

Merge branch 'master' into static-service

9af7333

Merge branch 'master' into static-service

58c67f3

# Conflicts: # server.js # services/base.js

Clean lint

ce5ded3

Run prettier

8303a3a

Fixes

882e42c

Restore old cache behavior + apply to other static badge

d166540

Tidy up service tester

df2feec

Rework this again

c6e0115

Formatting

8d7b638

Fixes

a19fcff

Use a subclass

7656ba3

Clean diff

964b7d5

Clean lint

bc26948

paulmelnikow added service-badge Accepted and actionable changes, features, and bugs core Server, BaseService, GitHub auth labels Nov 7, 2018

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 7, 2018 02:09 Inactive

paulmelnikow added 3 commits November 6, 2018 21:12

Clean diff

76c22b2

+ Inline comment

a4b7241

Clean diff

d574f24

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 7, 2018 02:16 Inactive

paulmelnikow commented Nov 7, 2018

View reviewed changes

Merge branch 'master' into static-service

aab5906

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 11, 2018 17:24 Inactive

Merge branch 'master' into static-service

a4ae95d

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 14, 2018 22:36 Inactive

paulmelnikow mentioned this pull request Nov 15, 2018

[badge-json] new "badge-json" service #1525

Closed

paulmelnikow added the blocker PRs and epics which block other work label Nov 15, 2018

chris48s previously approved these changes Nov 16, 2018

View reviewed changes

Merge branch 'master' into static-service

704d231

paulmelnikow had a problem deploying to shields-staging-pr-2284 November 16, 2018 21:52 Failure

paulmelnikow added 2 commits November 16, 2018 16:57

Update based on lookup table measurement

1144f00

Update based on gh-badges move

2731086

paulmelnikow dismissed chris48s’s stale review via 2731086 November 16, 2018 21:59

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 16, 2018 21:59 Inactive

Fix all one color badge

9a36161

paulmelnikow temporarily deployed to shields-staging-pr-2284 November 16, 2018 22:28 Inactive

chris48s approved these changes Nov 16, 2018

View reviewed changes

Merge branch 'master' into static-service

d7c5335

paulmelnikow deployed to shields-staging-pr-2284 November 17, 2018 00:19 View deployment

paulmelnikow merged commit 065dd57 into master Nov 17, 2018

paulmelnikow deleted the static-service branch November 17, 2018 00:21

paulmelnikow added a commit that referenced this pull request Nov 17, 2018

Allow handle() to return a numeric message

c4c5d35

This regression from #2284 was causing `{ message: 22 }` to render as `’n/a’`, as in this test run: https://circleci.com/gh/badges/shields/23680

paulmelnikow added a commit that referenced this pull request Nov 17, 2018

Allow handle() to return a numeric message

4891b45

This regression from #2284 was causing `{ message: 22 }` to render as `’n/a’`, as in this test run: https://circleci.com/gh/badges/shields/23680

paulmelnikow mentioned this pull request Nov 17, 2018

Allow handle() to return a numeric message #2332

Merged

paulmelnikow mentioned this pull request Nov 17, 2018

Deprecate old [StaticBadge] using a redirect #2333

Merged

paulmelnikow added a commit that referenced this pull request Nov 17, 2018

Allow handle() to return a numeric message (#2332)

547380f

This regression from #2284 was causing `{ message: 22 }` to render as `’n/a’`, as in this test run: https://circleci.com/gh/badges/shields/23680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move [StaticBadge] to own service & add test; also affects [gitter] #2284

Move [StaticBadge] to own service & add test; also affects [gitter] #2284

paulmelnikow commented Nov 7, 2018

shields-ci commented Nov 7, 2018 •

edited

Loading

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

paulmelnikow Nov 7, 2018

chris48s commented Nov 16, 2018

paulmelnikow commented Nov 16, 2018

shields-deployment bot commented Nov 17, 2018

Move [StaticBadge] to own service & add test; also affects [gitter] #2284

Move [StaticBadge] to own service & add test; also affects [gitter] #2284

Conversation

paulmelnikow commented Nov 7, 2018

shields-ci commented Nov 7, 2018 • edited Loading

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

paulmelnikow Nov 7, 2018

Choose a reason for hiding this comment

chris48s commented Nov 16, 2018

paulmelnikow commented Nov 16, 2018

shields-deployment bot commented Nov 17, 2018

shields-ci commented Nov 7, 2018 •

edited

Loading