Skip to content

Commit 8564e52

Browse files
committed
Get rid of redundant env variable
1 parent e6b011f commit 8564e52

5 files changed

Lines changed: 29 additions & 25 deletions

File tree

src/deploy/apphosting/args.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
import { AppHostingSingle } from "../../firebaseConfig";
2-
import { BuildConfig, Env } from "../../gcp/apphosting";
2+
import { BuildConfig } from "../../gcp/apphosting";
33

44
export interface LocalBuild {
55
buildConfig: BuildConfig;
66
buildDir: string;
77
annotations: Record<string, string>;
8-
env: Env[];
98
}
109

1110
export interface Context {

src/deploy/apphosting/deploy.spec.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ function initializeContext(): Context {
4444
buildDir: "./nextjs/standalone",
4545
buildConfig: {},
4646
annotations: {},
47-
env: [],
4847
},
4948
},
5049
};

src/deploy/apphosting/prepare.spec.ts

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import * as managementApps from "../../management/apps";
1414
import * as experiments from "../../experiments";
1515
import { FirebaseError } from "../../error";
1616
import * as projectUtils from "../../getProjectNumber";
17-
import * as experiments from "../../experiments";
1817

1918
const BASE_OPTS = {
2019
cwd: "/",
@@ -111,7 +110,6 @@ describe("apphosting", () => {
111110
buildConfig,
112111
annotations,
113112
});
114-
sinon.stub(experiments, "assertEnabled").returns();
115113
listBackendsStub.onFirstCall().resolves({
116114
backends: [
117115
{
@@ -133,7 +131,6 @@ describe("apphosting", () => {
133131
buildDir: "./next/standalone",
134132
buildConfig,
135133
annotations,
136-
env: [],
137134
});
138135
expect(backend.ensureAppHostingServiceAgentRoles).to.have.been.called;
139136
});
@@ -211,9 +208,7 @@ describe("apphosting", () => {
211208
};
212209
const context = initializeContext();
213210

214-
sinon
215-
.stub(experiments, "assertEnabled")
216-
.throws(new Error("Experiment 'apphostinglocalbuilds' is not enabled."));
211+
assertEnabledStub.throws(new Error("Experiment 'apphostinglocalbuilds' is not enabled."));
217212
listBackendsStub.onFirstCall().resolves({
218213
backends: [
219214
{

src/deploy/apphosting/prepare.ts

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -196,12 +196,10 @@ export default async function (context: Context, options: Options): Promise<void
196196
managementApps.AppPlatform.WEB,
197197
)) as WebConfig;
198198
const autoinitVars = getAutoinitEnvVars(webappConfig);
199-
buildEnv[cfg.backendId] = {
200-
...buildEnv[cfg.backendId],
201-
...Object.fromEntries(
202-
Object.entries(autoinitVars).map(([key, value]) => [key, { value }]),
203-
),
204-
};
199+
// TODO: confirm the correct priority of auto-init variables
200+
for (const [key, value] of Object.entries(autoinitVars)) {
201+
buildEnv[cfg.backendId][key] = { value };
202+
}
205203
} catch (e) {
206204
logLabeledWarning(
207205
"apphosting",
@@ -221,11 +219,12 @@ export default async function (context: Context, options: Options): Promise<void
221219
);
222220
}
223221
context.backendLocalBuilds[cfg.backendId] = {
224-
// TODO(9114): This only works for nextjs.
225222
buildDir: outputFiles[0],
226-
buildConfig,
223+
buildConfig: {
224+
...buildConfig,
225+
env: mergeEnvVars(buildConfig.env || [], runtimeEnv[cfg.backendId] || []),
226+
},
227227
annotations,
228-
env: runtimeEnv[cfg.backendId] || [],
229228
};
230229
} catch (e: unknown) {
231230
const errorMsg = e instanceof Error ? e.message : String(e);
@@ -268,3 +267,21 @@ export function getBackendConfigs(options: Options): AppHostingMultiple {
268267
}
269268
return backendConfigs.filter((cfg) => backendIds.includes(cfg.backendId));
270269
}
270+
271+
/**
272+
* Merges two lists of environment variables, giving precedence to the values in overrides.
273+
*/
274+
function mergeEnvVars(base: Env[], overrides: Env[]): Env[] {
275+
const merged = new Map<string, Env>();
276+
for (const env of base) {
277+
if (env.variable) {
278+
merged.set(env.variable, env);
279+
}
280+
}
281+
for (const env of overrides) {
282+
if (env.variable) {
283+
merged.set(env.variable, env);
284+
}
285+
}
286+
return Array.from(merged.values());
287+
}

src/deploy/apphosting/release.ts

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,7 @@ export default async function (context: Context, options: Options): Promise<void
5151
location: context.backendLocations[backendId],
5252
buildInput: {
5353
// TODO: Confirm this config behavior
54-
config: {
55-
...context.backendLocalBuilds[backendId]?.buildConfig,
56-
env: [
57-
...(context.backendLocalBuilds[backendId]?.buildConfig?.env || []),
58-
...(context.backendLocalBuilds[backendId]?.env || []),
59-
],
60-
},
54+
config: context.backendLocalBuilds[backendId]?.buildConfig || {},
6155
source: {
6256
archive: {
6357
userStorageUri: context.backendStorageUris[backendId],

0 commit comments

Comments
 (0)