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.

Sign up for GitHub

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

Jump to bottom

Expose stack on functions.yaml instead of stack.yaml. #1036

Merged
merged 3 commits into from
Feb 15, 2022
Merged

Expose stack on functions.yaml instead of stack.yaml. #1036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f012976
Expose stack on functions.yaml instead of stack.yaml.
taeold Feb 9, 2022
effa458
Prettier.
taeold Feb 9, 2022
9fd8611
Merge branch 'master' into dl-cf3-cc-fixes
taeold Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions scripts/bin-test/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ async function startBin(
env: {
PATH: process.env.PATH,
GLCOUD_PROJECT: 'test-project',
STACK_CONTROL_API_PORT: port,
PORT: port,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for changing this to a more generic name? Isn't it likely that users might have this var set for other processes.

Copy link
Member

Choose a reason for hiding this comment

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

The original design used two ports: one for the general API and one for the "control" API. We discovered that Python cannot handle two ports on the same process, so we are consolidating on one port. $PORT is the standard env variable for Cloud Run, so we're using it too.

Copy link

Choose a reason for hiding this comment

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

Thank you for your service.

Copy link

Choose a reason for hiding this comment

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

Is ok

FUNCTIONS_CONTROL_API: 'true',
},
});

Expand All @@ -95,7 +96,7 @@ async function startBin(

await retryUntil(async () => {
try {
await fetch(`http://localhost:${port}/__/stack.yaml`);
await fetch(`http://localhost:${port}/__/functions.yaml`);
} catch (e) {
if (e?.code === 'ECONNREFUSED') {
return false;
Expand Down Expand Up @@ -132,7 +133,7 @@ async function startBin(
};
}

describe('stack.yaml', () => {
describe('functions.yaml', () => {
async function runTests(tc: Testcase) {
let port: number;
let cleanup: () => Promise<void>;
Expand All @@ -147,14 +148,14 @@ describe('stack.yaml', () => {
await cleanup?.();
});

it('stack.yaml returns expected Manifest', async () => {
const res = await fetch(`http://localhost:${port}/__/stack.yaml`);
it('functions.yaml returns expected Manifest', async () => {
const res = await fetch(`http://localhost:${port}/__/functions.yaml`);
const text = await res.text();
let parsed: any;
try {
parsed = yaml.load(text);
} catch (err) {
throw new Error('Failed to parse stack.yaml ' + err);
throw new Error('Failed to parse functions.yaml ' + err);
}
expect(parsed).to.be.deep.equal(tc.expected);
});
Expand Down
29 changes: 16 additions & 13 deletions src/bin/firebase-functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,24 @@ async function handleQuitquitquit(req: express.Request, res: express.Response) {

app.get('/__/quitquitquit', handleQuitquitquit);
app.post('/__/quitquitquit', handleQuitquitquit);
app.get('/__/stack.yaml', async (req, res) => {
try {
const stack = await loadStack(functionsDir);
res.setHeader('content-type', 'text/yaml');
res.send(JSON.stringify(stack));
} catch (e) {
res
.status(400)
.send(`Failed to generate manifest from function source: ${e}`);
}
});

if (process.env.FUNCTIONS_CONTROL_API === 'true') {
app.get('/__/functions.yaml', async (req, res) => {
try {
const stack = await loadStack(functionsDir);
res.setHeader('content-type', 'text/yaml');
res.send(JSON.stringify(stack));
Copy link
Member

Choose a reason for hiding this comment

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

Lol. I guess JSON is technically YAML.

} catch (e) {
res
.status(400)
.send(`Failed to generate manifest from function source: ${e}`);
}
});
}

let port = 8080;
if (process.env.STACK_CONTROL_API_PORT) {
port = Number.parseInt(process.env.STACK_CONTROL_API_PORT);
if (process.env.PORT) {
port = Number.parseInt(process.env.PORT);
}

console.log('Serving at port', port);
Expand Down
3 changes: 1 addition & 2 deletions src/runtime/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,9 @@ export async function loadStack(functionsDir: string): Promise<ManifestStack> {

extractStack(mod, endpoints, requiredAPIs);

const stack: ManifestStack = {
return {
endpoints,
specVersion: 'v1alpha1',
requiredAPIs: mergeRequiredAPIs(requiredAPIs),
};
return stack;
}