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

feat: allow setting environment variables and disable extending in DenoServer #82

Merged
merged 11 commits into from
Aug 9, 2022
22 changes: 14 additions & 8 deletions src/bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ interface ProcessRef {

interface RunOptions {
pipeOutput?: boolean
env?: NodeJS.ProcessEnv
extendEnv?: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

In what case would we set extendEnv to true? Or is this for backwards compatibility?

Copy link
Contributor Author

@danez danez Aug 4, 2022

Choose a reason for hiding this comment

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

The default value is true atm. I was hardcoding this at first, but then wondered if runInBackground might be used in other scenarios in the future. Do you think I should hardcode this to false?
And because run does the same, besides the background part I also added the options there.

}

class DenoBridge {
Expand Down Expand Up @@ -190,8 +192,8 @@ class DenoBridge {
return { global: false, path: downloadedPath }
}

getEnvironmentVariables() {
const env: Record<string, string> = {}
getEnvironmentVariables(inputEnv: NodeJS.ProcessEnv = {}) {
const env: NodeJS.ProcessEnv = { ...inputEnv }

if (this.denoDir !== undefined) {
env.DENO_DIR = this.denoDir
Expand All @@ -202,20 +204,24 @@ class DenoBridge {

// Runs the Deno CLI in the background and returns a reference to the child
// process, awaiting its execution.
async run(args: string[], { pipeOutput }: RunOptions = {}) {
async run(args: string[], { pipeOutput, env: inputEnv, extendEnv = true }: RunOptions = {}) {
const { path: binaryPath } = await this.getBinaryPath()
const env = this.getEnvironmentVariables()
const options = { env }
const env = this.getEnvironmentVariables(inputEnv)
const options: Options = { env, extendEnv }

return DenoBridge.runWithBinary(binaryPath, args, options, pipeOutput)
}

// Runs the Deno CLI in the background, assigning a reference of the child
// process to a `ps` property in the `ref` argument, if one is supplied.
async runInBackground(args: string[], pipeOutput?: boolean, ref?: ProcessRef) {
async runInBackground(
args: string[],
ref?: ProcessRef,
{ pipeOutput, env: inputEnv, extendEnv = true }: RunOptions = {},
) {
const { path: binaryPath } = await this.getBinaryPath()
const env = this.getEnvironmentVariables()
const options = { env }
const env = this.getEnvironmentVariables(inputEnv)
const options: Options = { env, extendEnv }
const ps = DenoBridge.runWithBinary(binaryPath, args, options, pipeOutput)

if (ref !== undefined) {
Expand Down
11 changes: 9 additions & 2 deletions src/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const prepareServer = ({
port,
}: PrepareServerOptions) => {
const processRef: ProcessRef = {}
const startIsolate = async (newFunctions: EdgeFunction[]) => {
const startIsolate = async (newFunctions: EdgeFunction[], env: NodeJS.ProcessEnv = {}) => {
if (processRef?.ps !== undefined) {
await killProcess(processRef.ps)
}
Expand Down Expand Up @@ -60,7 +60,14 @@ const prepareServer = ({

const bootstrapFlags = ['--port', port.toString()]

await deno.runInBackground(['run', ...denoFlags, stage2Path, ...bootstrapFlags], true, processRef)
// We set `extendEnv: false` to avoid polluting the edge function context
// with variables from the user's system, since those will not be available
// in the production environment.
await deno.runInBackground(['run', ...denoFlags, stage2Path, ...bootstrapFlags], processRef, {
pipeOutput: true,
env,
extendEnv: false,
})

const success = await waitForServer(port, processRef.ps)

Expand Down
141 changes: 141 additions & 0 deletions test/bridge.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import { Buffer } from 'buffer'
import fs from 'fs'
import { createRequire } from 'module'
import { platform, env } from 'process'
import { PassThrough } from 'stream'

import test from 'ava'
import nock from 'nock'
import { spy } from 'sinon'
import tmp, { DirectoryResult } from 'tmp-promise'

import { DenoBridge } from '../src/bridge.js'
import { getPlatformTarget } from '../src/platform.js'

const require = createRequire(import.meta.url)
const archiver = require('archiver')

const getMockDenoBridge = function (tmpDir: DirectoryResult, mockBinaryOutput: string) {
const latestVersion = '1.20.3'
const data = new PassThrough()
const archive = archiver('zip', { zlib: { level: 9 } })

archive.pipe(data)
archive.append(Buffer.from(mockBinaryOutput.replace(/@@@latestVersion@@@/g, latestVersion)), {
name: platform === 'win32' ? 'deno.exe' : 'deno',
})
archive.finalize()

const target = getPlatformTarget()

nock('https://dl.deno.land').get('/release-latest.txt').reply(200, `v${latestVersion}`)
nock('https://dl.deno.land')
.get(`/release/v${latestVersion}/deno-${target}.zip`)
.reply(200, () => data)

const beforeDownload = spy()
const afterDownload = spy()

return new DenoBridge({
cacheDirectory: tmpDir.path,
onBeforeDownload: beforeDownload,
onAfterDownload: afterDownload,
useGlobal: false,
})
}

test.serial('Does not inherit environment variables if `extendEnv` is false', async (t) => {
const tmpDir = await tmp.dir()
const deno = getMockDenoBridge(
tmpDir,
`#!/usr/bin/env sh

if [ "$1" = "test" ]
then
env
else
echo "deno @@@latestVersion@@@"
fi`,
)

// The environment sets some variables so let us see what they are and remove them from the result
const referenceOutput = await deno.run(['test'], { env: {}, extendEnv: false })
env.TADA = 'TUDU'
const result = await deno.run(['test'], { env: { LULU: 'LALA' }, extendEnv: false })
let output = result?.stdout ?? ''

delete env.TADA

referenceOutput?.stdout.split('\n').forEach((line) => {
output = output.replace(line.trim(), '')
})
output = output.trim().replace(/\n+/, '\n')

t.is(output, 'LULU=LALA')

await fs.promises.rmdir(tmpDir.path, { recursive: true })
})

test.serial('Does inherit environment variables if `extendEnv` is true', async (t) => {
const tmpDir = await tmp.dir()
const deno = getMockDenoBridge(
tmpDir,
`#!/usr/bin/env sh

if [ "$1" = "test" ]
then
env
else
echo "deno @@@latestVersion@@@"
fi`,
)

// The environment sets some variables so let us see what they are and remove them from the result
const referenceOutput = await deno.run(['test'], { env: {}, extendEnv: true })
env.TADA = 'TUDU'
const result = await deno.run(['test'], { env: { LULU: 'LALA' }, extendEnv: true })
let output = result?.stdout ?? ''

delete env.TADA

referenceOutput?.stdout.split('\n').forEach((line) => {
output = output.replace(line.trim(), '')
})
output = output.trim().replace(/\n+/, '\n')

t.is(output, 'LULU=LALA\nTADA=TUDU')

await fs.promises.rmdir(tmpDir.path, { recursive: true })
})

test.serial('Does inherit environment variables if `extendEnv` is not set', async (t) => {
const tmpDir = await tmp.dir()
const deno = getMockDenoBridge(
tmpDir,
`#!/usr/bin/env sh

if [ "$1" = "test" ]
then
env
else
echo "deno @@@latestVersion@@@"
fi`,
)

// The environment sets some variables so let us see what they are and remove them from the result
const referenceOutput = await deno.run(['test'], { env: {}, extendEnv: true })
env.TADA = 'TUDU'
const result = await deno.run(['test'], { env: { LULU: 'LALA' } })
let output = result?.stdout ?? ''

delete env.TADA

referenceOutput?.stdout.split('\n').forEach((line) => {
output = output.replace(line.trim(), '')
})
output = output.trim().replace(/\n+/, '\n')

t.is(output, 'LULU=LALA\nTADA=TUDU')

await fs.promises.rmdir(tmpDir.path, { recursive: true })
})
2 changes: 1 addition & 1 deletion test/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const archiver = require('archiver')

test('Downloads the Deno CLI on demand and caches it for subsequent calls', async (t) => {
const latestVersion = '1.20.3'
const mockBinaryOutput = `#!/usr/bin/env node\n\nconsole.log("deno ${latestVersion}")`
const mockBinaryOutput = `#!/usr/bin/env sh\n\necho "deno ${latestVersion}"`
const data = new PassThrough()
const archive = archiver('zip', { zlib: { level: 9 } })

Expand Down