This implements the configuration mechanism I described in #2621. The heavy lifting is delegated to [node-config](https://github.com/lorenwest/node-config) with a minor assist from [dotenv](https://github.com/motdotla/dotenv).
`private/secret.json` has been replaced with environment variables and/or `config/local.yml`. See `doc/server-secrets.md`.
Because `server.js` was long a monolith, there are a bunch of shims in place to facilitate unit testing. A few of the test suites share port 1111 which means if one of them fails to set up, the port won't be freed and other unrelated tests will fail. Some of the tests which trigger server setup include timeouts which were added to give setup code time to run. In one the test suites, we actually modify `process.argv`, which seems completely gross.
This implements a few changes which improve this:
1. Separate the server from the server startup script, splitting out `lib/server.js`.
2. Inject config into the server and validate the config schema.
3. Inject config into the service test runner.
4. Use `portfinder`, a popular utility for grabbing open ports during testing.
5. Switch more of the setup code from callbacks to async-await.
Overall it leaves everything acting more reliably and looking rather cleaner, if in a few places more verbose.
It also fixes the root cause of #1455, a `setTimeout` in `rate-limit`. Off and on during development of this changeset, Mocha would decide not to exit, and that turned out to be the culprit.
Fix#1455
This starts the rewrite of the dynamic badges. I've pulled into BaseService an initial version of the query param validation from #2325.
I've extended from BaseJsonService to avoid duplicating the deserialization logic, though it means there is a bit of duplicated code among the three dynamic services. The way to unravel this would be to move the logic from `_requestJson` and friends from the base classes into functions so DynamicJson can inherit from BaseDynamic. Would that be worth it?
This introduces a regression of #1446 for this badge.
Close#2345
There's a lot going on in this PR, though it's all interdependent, so the only way I can see to break it up into smaller pieces would be serially.
1. I completely refactored the functions for managing cache headers. These have been added to `services/cache-headers.js`, and in some ways set the stage for the rest of this PR.
- There are ample higher-level test of the functionality via `request-handler`. Refactoring these tests was deferred. Cache headers were previously dealt with in three places:
- `request-handler.js`, for the dynamic badges. This function now calls `setCacheHeaders`.
- `base-static.js`, for the static badges. This method now calls the wordy `serverHasBeenUpSinceResourceCached` and `setCacheHeadersForStaticResource`.
- The bitFlip badge in `server.js`. 👈 This is what set all this in motion. This badge has been refactored to a new-style service based on a new `NoncachingBaseService` which does not use the Shields in-memory cache that the dynamic badges user.
- I'm open to clearer names for `NoncachingBaseService`, which is kind of terrible. Absent alternatives, I wrote a short essay of clarification in the docstring. 😝
2. In the process of writing `NoncachingBaseService`, I discovered it takes several lines of code to instantiate and invoke a service. These would be duplicated in three or four places in production code, and in lots and lots of tests. I kept the line that goes from regex to namedParams (for reasons) and moved the rest into a static method called `invoke()`, which instantiates and invokes the service. This _replaced_ the instance method `invokeHandler`.
- I gently reworked the unit tests to use `invoke` instead of `invokeHandler`– generally for the better.
- I made a small change to `BaseStatic`. Now it invokes `handle()` async as the dynamic badges do. This way it could use `BaseService.invoke()`.
3. There was logic in `request-handler` for processing environment variables, validating them, and setting defaults. This could have been lifted whole-hog to `services/cache-headers.js`, though I didn't do that. Instead I moved it to `server-config.js`. Ideally `server-config` is the only module that should access `process.env`. This puts the defaults and config validation in one place, decouples the config schema from the entire rest of the application, and significantly simplifies our ability to test different configs, particularly on small units of code. (We were doing this well enough before in `request-handler.spec`, though it required mutating the environment, which was kludgy.) Some of the `request-handler` tests could be rewritten at a higher level, with lower-level data-driven tests directly against `cache-headers`.
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.
This picks up @RedSparr0w's work in #1802.
1. 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.
2. Most of the custom static-badge logic is in a BaseStaticBadge base class.
3. Rewrite the static Gitter badge to use BaseStaticBadge.
4. A bit of minor cleanup in related functions.
This simplifies and further optimizes text-width computation by computing the entire width table in advance, and serializing it in the style of QuickTextMeasurer (#1390). This entirely removes the need for PDFKit at runtime. This has the advantage of fixing #1305 – more generally: producing the same result everywhere – without having to deploy a copy of Verdana.
The lifting is delegated to these three libraries, which are housed in a monorepo: https://github.com/metabolize/anafanafo
I'd be happy to move it into the badges org if folks want to collaborate on maintaining them.
QuickTextMeasurer took kerning pairs into account, whereas this implementation does not. I was thinking kerning would be a necessary refinement, though this seems to work well enough.
I dropped in a binary-search package to traverse the data structure, in part to conserve space. This causes a moderate performance regression, though there is ample room for improving on that: https://github.com/badges/shields/pull/2311#issuecomment-439182704
* move gh-badges files out of /lib
As far as possible, this is just moving files
around and updating paths however there are 2
functional changes in this commit:
- remove use of lib/register-chai-plugins.spec
in badge-cli.spec.js
- remove use of starRating()
in text-measurer.spec.js
* update service tests that use colorscheme.json
* split package.json in two
* clean up import
* don't hard-code path
* start a changelog
* put a license file in the package dir
* re-organise documentation 📚
* don't pack test files
* remove favicon from Makefile
* give package its own test command
* link the docs better in README
* Basic process metrics
* Enable Prometheus by an environment variable
* Code formatting
* Documentation for Prometheus metrics
* Link from README to documentation of Prometheus
* Link from README to documentation of Prometheus
* Link from README to documentation of Prometheus
* Separate module for metrics + tests
* Metrics limited by IP
* Metrics are forbidded for all requets by default
* Code refactoring
* allowedIps passed as a string to PrometheusMetrics
* Handle missing config
* METRICS_PROMETHEUS_ALLOWED_IPS added to documentation
* Log info about enabled metrics
* Unused code removed
* package-lock.json updated
* prom-client updated to 11.1.2
* Code refactoring
* Do not read IP address from X-Forwarder-For header
This is consistent with what we're pretty much already doing, and saves us from making the request during code review.
These were all autofixed and most of them seem easier to read. Some in the legacy services should be rewritten in more legible forms during refactor (ie using intermediate variables, or using request’s qs option). There are some in helper functions and elsewhere that should get rewritten separately. I don't want to change them in this PR because the changes will get lost in this diff, though we could identify them here and fix them before or just after.
This creates a new convenience class which consolidates all the Github initialization. It supports dependency injection and facilitates refactoring the persistence along the lines of #1205.
Also ref #1848
When JSON responses come back, they are sometimes not in the format expected by the API. As a result we have a lot of defensive coding (expressions like `(data || {}).someProperty`) to avoid exceptions being thrown in badge processing. Often we rely on the `try` blocks that wrap so much of the badge-processing code, which catch all JavaScript exceptions and return some error result, usually **invalid**. The problem with this is that these `try` blocks catch all sorts of programmer errors too, so when we see **invalid** we don't know whether the API returned something unexpected, or we've made a mistake. We also spend a lot of time writing defensive tests around malformed responses, and creating and maintaining the defensive coding.
A better solution is to validate the API responses using declarative contracts. Here the programmer says exactly what they expect from the API. That way, if the response isn't what we expect we can just say it's an **invalid json response**. And if our code then throws an exception, well that's our mistake; when we catch that we can call it a **shields internal error**. It's also less code and less error-prone. Over time we may be confident enough in the contracts that we won't need so many tests of malformed responses. The contract doesn't need to describe the entire response, only the part that's needed. Unknown keys can simply be dropped, preventing unvalidated parts of the response from creeping into the code. Checking what's in our response before calling values on it also makes our code more secure.
I used Joi here, since we're already using it for testing. There may be another contracts library that's a better fit, though I think we could look at that later.
Those changes are in base.js.
The rest is a rewrite of the remaining NPM badges, including the extraction of an NpmBase class. Inspired by @chris48s's work in #1740, this class splits the service concerns into fetching, validation, transformation, and rendering. This is treated as a design pattern. See the PR discussion for more. There are two URL patterns, one which allows specifying a tag (used by e.g. the version badge `https://img.shields.io/npm/v/npm/next.svg`), and the other which does not accept a tag (e.g. the license badge `https://img.shields.io/npm/l/express.svg`). Subclasses like NpmLicense and NpmTypeDefinitions can specify the URL fragment, examples, the validation schema for the chunk of the package data they use, and a render function. The NpmVersion subclass uses a different endpoint, so it overrides the `handle` implementation from NpmBase.
The remaining services using BaseJsonService are shimmed, so they will keep working after the changes.
* loosely detect semver versions
* semver loose validity
* add test
* semver compare versions when null
* add comments for loose
* remove console.log, only match semver scheme
* looser version restrictions
* re-sort version tests
comments refered to incorrect test
* add support for pre-release tags
#1682
* support uppercase, lowercase mixed
* make pre-release opt in
* change pre param to object
* add notes
* hide left side if no text
* fix whitespace at start of template
* for-the-badge done
* center text
* add logo support
* update snapshot test
* make first - in static badge optional
* add no label support to non static badges
* Add test for label
* update to not allow use of false/0/null
* fixup
* add support for rgb, rgb, css named colors
* add support for hsl, hsla & add color-validate
* update makeBadge test, better coverage
* re-add comment
* add comment for supported colors
* dynamic badge gen, remove 'hex'
* add support for 1.0 opacity & fix 101-109
* fix colorscheme tests
* remove extra tests
* add test for negative values
* add test for uppercase & mixed case colors
* fix mixed case/uppercase test
* allow whitespace around color
* update test error messages
* add comments
* add more uppercase test
* update error message
* default to grey/red if invalid color chosen
default colorscheme:
colorA: grey
colorB: red
* Revert "default to grey/red if invalid color chosen"
This reverts commit 10db0c6d74.
Reverted as this affects the CLI version/when no color specified.
* validColor -> isCSSColor
* assignColor function
* update tests to use sazerac
* Add Jenkins Jacoco coverage badge
* [Jenkins] add service test for jacoco coverage
* Added Jenkins Coverage (Jacoco) in all-badge-examples page
* Defined variables using let/const instead of var
* Used template string for the uri
* Used checkErrorResponse helper function for the error check
* Used NaN method for not a number test
* Prefixed the original Jenkin coverage test with "cobertura:"
* Moved the happy test case at front
* Merge the business logic between jacoco and cobertura
* Fixed lint issue
* Trigger notification
