Skip to content

Commit

Permalink
feat: proxy support (#681)
Browse files Browse the repository at this point in the history
* test: rename test for clarity

* chore: add lint rule to prevent raw node-fetch use

also reformatting eslintrc and adding contrib docs on this change

* feat: add proxy support

* chore: fix TOC
  • Loading branch information
kanadgupta authored Nov 29, 2022
1 parent 0166c1e commit 0e4644a
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 16 deletions.
35 changes: 24 additions & 11 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{
"extends": [
"@readme/eslint-config",
"@readme/eslint-config/typescript"
],
"extends": ["@readme/eslint-config", "@readme/eslint-config/typescript"],
"root": true,
"parserOptions": {
"ecmaVersion": 2020
Expand All @@ -16,13 +13,16 @@
}
],
"rules": {
"@typescript-eslint/ban-types": ["error", {
"types": {
// We specify `{}` in `CommandOptions` generics when those commands don't have their own
// options and it's cleaner for us to do that than `Record<string, unknown>`.
"{}": false
"@typescript-eslint/ban-types": [
"error",
{
"types": {
// We specify `{}` in `CommandOptions` generics when those commands don't have their own
// options and it's cleaner for us to do that than `Record<string, unknown>`.
"{}": false
}
}
}],
],

/**
* Because our command classes have a `run` method that might not always call `this` we need to
Expand All @@ -45,6 +45,19 @@
*/
"no-console": "warn",

"no-restricted-syntax": "off"
"no-restricted-syntax": "off",

"no-restricted-imports": [
"error",
{
"paths": [
{
"name": "node-fetch",
"importNames": ["default"],
"message": "Avoid using `node-fetch` directly and instead use the fetch wrapper located in `lib/fetch.ts`. See CONTRIBUTING.md for more information."
}
]
}
]
}
}
6 changes: 6 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ When writing command logic, avoid using `console` statements (and correspondingl

<img align="right" width="25%" style="margin-bottom: 2em" src="https://owlbert.io/images/owlberts-png/Blocks.psd.png">

### Making `fetch` requests

`fetch` requests are very common in this codebase. When sending `fetch` requests to the ReadMe API (i.e., [dash.readme.com](https://dash.readme.com)), make sure to use the `fetch` wrapper function located in [`src/lib/fetch.ts`](src/lib/fetch.ts). We have an ESLint rule to flag this.

In that wrapper function, we set several important request headers and configure the proxy, if the user added one via `HTTPS_PROXY`.

### Commit Conventions

For our general commit conventions, please consult our organization contributing guidelines [here](https://github.com/readmeio/.github/blob/main/.github/CONTRIBUTING.md#commit-conventions).
10 changes: 10 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ https://github.com/jonschlinkert/markdown-toc/issues/119
- [CLI Configuration](#cli-configuration)
- [Setup](#setup)
- [Authentication](#authentication)
- [Proxy](#proxy)
- [GitHub Actions Configuration](#github-actions-configuration)
- [Usage](#usage)
- [Common `rdme` Options](#common-rdme-options)
Expand Down Expand Up @@ -94,6 +95,15 @@ For usage in CI environments (GitHub Actions, CircleCI, Travis CI, etc.) or if y

`rdme whoami` is also available to you to determine who is logged in, and to what project. You can clear your stored credentials with `rdme logout`.

### Proxy

`rdme` makes API requests to the ReadMe API, which is located at [dash.readme.com](https://dash.readme.com). If you need to configure a proxy for these requests, you can do so by setting the `HTTPS_PROXY` environmental variable.

```sh
export HTTPS_PROXY=https://proxy.example.com:5678
rdme openapi
```

## GitHub Actions Configuration

> **Note**
Expand Down
9 changes: 7 additions & 2 deletions __tests__/helpers/get-api-mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@ import nock from 'nock';

import { getUserAgent } from '../../src/lib/fetch';

export default function getAPIMock(reqHeaders = {}) {
return nock(config.get('host'), {
/**
* Nock wrapper that adds required `user-agent` request header
* so it gets properly picked up by nock.
* @param proxy Optional proxy URL. Must contain trailing slash.
*/
export default function getAPIMock(reqHeaders = {}, proxy = '') {
return nock(`${proxy}${config.get('host')}`, {
reqheaders: {
'User-Agent': getUserAgent(),
...reqHeaders,
Expand Down
45 changes: 44 additions & 1 deletion __tests__/lib/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('#fetch()', () => {
mock.done();
});

it('should support if we dont supply any other options with the request', async () => {
it('should make fetch call if no other request options are provided', async () => {
const mock = getAPIMock()
.get('/api/v1/doesnt-need-auth')
.reply(200, function () {
Expand All @@ -105,6 +105,49 @@ describe('#fetch()', () => {
expect(headers['x-github-sha']).toBeUndefined();
mock.done();
});

describe('proxies', () => {
afterEach(() => {
delete process.env.https_proxy;
delete process.env.HTTPS_PROXY;
});

it('should support proxies via HTTPS_PROXY env variable', async () => {
const proxy = 'https://proxy.example.com:5678';

process.env.HTTPS_PROXY = proxy;

const mock = getAPIMock({}, `${proxy}/`).get('/api/v1/proxy').reply(200);

await fetch(`${config.get('host')}/api/v1/proxy`);

expect(mock.isDone()).toBe(true);
});

it('should support proxies via https_proxy env variable', async () => {
const proxy = 'https://proxy.example.com:5678';

process.env.https_proxy = proxy;

const mock = getAPIMock({}, `${proxy}/`).get('/api/v1/proxy').reply(200);

await fetch(`${config.get('host')}/api/v1/proxy`);

expect(mock.isDone()).toBe(true);
});

it('should handle trailing slash in proxy URL', async () => {
const proxy = 'https://proxy.example.com:5678/';

process.env.https_proxy = proxy;

const mock = getAPIMock({}, proxy).get('/api/v1/proxy').reply(200);

await fetch(`${config.get('host')}/api/v1/proxy`);

expect(mock.isDone()).toBe(true);
});
});
});

describe('#cleanHeaders()', () => {
Expand Down
17 changes: 15 additions & 2 deletions src/lib/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { RequestInit, Response } from 'node-fetch';

import mime from 'mime-types';
// eslint-disable-next-line no-restricted-imports
import nodeFetch, { Headers } from 'node-fetch';

import pkg from '../../package.json';
Expand All @@ -11,6 +12,16 @@ import { debug } from './logger';

const SUCCESS_NO_CONTENT = 204;

function getProxy() {
// this is something of an industry standard env var, hence the checks for different casings
const proxy = process.env.HTTPS_PROXY || process.env.https_proxy;
if (proxy) {
// adds trailing slash
return proxy.endsWith('/') ? proxy : `${proxy}/`;
}
return '';
}

/**
* Getter function for a string to be used in the user-agent header based on the current
* environment.
Expand Down Expand Up @@ -46,9 +57,11 @@ export default function fetch(url: string, options: RequestInit = { headers: new

headers.set('x-readme-source', source);

debug(`making ${(options.method || 'get').toUpperCase()} request to ${url}`);
const fullUrl = `${getProxy()}${url}`;

debug(`making ${(options.method || 'get').toUpperCase()} request to ${fullUrl}`);

return nodeFetch(url, {
return nodeFetch(fullUrl, {
...options,
headers,
});
Expand Down
1 change: 1 addition & 0 deletions src/lib/getPkgVersion.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// eslint-disable-next-line no-restricted-imports
import fetch from 'node-fetch';

import pkg from '../../package.json';
Expand Down

0 comments on commit 0e4644a

Please sign in to comment.