-
Notifications
You must be signed in to change notification settings - Fork 956
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
Add firebase ext:sdk:install command #7581
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some style nits, but LGTM overall!
CHANGELOG.md
Outdated
@@ -5,3 +5,4 @@ | |||
- Improved handling of Spark projects in `firebase init dataconnect`. (#7666) | |||
- Updated Firebase Data Connect local toolkit version to v1.3.7, which adds support for `v1beta` gRPC APIs and the `OrderDirection` enum in Swift, and makes transactional queries and mutations opt-in with the `@transaction` directive. (#7679) | |||
- Add dataconnect SQL grant command `firebase dataconnect:sql:grant -R <role> -E email`. (#7656) | |||
- Added new command ext:sdk:install to allow you to configure extensions in a functions codebase. (#7581) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added new command ext:sdk:install to allow you to configure extensions in a functions codebase. (#7581) | |
- Added new command `firebase ext:sdk:install` to allow you to configure extensions in a functions codebase. (#7581) |
Nit: If there are docs, it would be good to link them here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/commands/ext-info.ts
Outdated
@@ -121,5 +122,10 @@ export const command = new Command("ext:info <extensionName>") | |||
`to install this extension, type ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`to install this extension, type ` + | |
`to install this extension, run ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/commands/ext-info.ts
Outdated
@@ -121,5 +122,10 @@ export const command = new Command("ext:info <extensionName>") | |||
`to install this extension, type ` + | |||
clc.bold(`firebase ext:install ${extensionName} --project=YOUR_PROJECT`), | |||
); | |||
utils.logLabeledBullet( | |||
logPrefix, | |||
`to install an autogenerated SDK for this extension into your functions codebase, type ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`to install an autogenerated SDK for this extension into your functions codebase, type ` + | |
`to install an autogenerated SDK for this extension into your functions codebase, run ` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -0,0 +1,111 @@ | |||
import * as clc from "colorette"; | |||
import { checkMinRequiredVersion } from "../checkMinRequiredVersion"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { checkMinRequiredVersion } from "../checkMinRequiredVersion"; | |
import { checkMinRequiredVersion } from "../checkMinRequiredVersion"; |
Nit: We usually put a newline between external and internal imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/deploy/extensions/planner.ts
Outdated
export async function haveDynamic(projectId: string): Promise<DeploymentInstanceSpec[]> { | ||
let instances = await extensionsApi.listInstances(projectId); | ||
// Only include instances created by SDK | ||
instances = instances.filter((i) => i.labels?.createdBy === "SDK"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optonal style nit: I like chaining these without setting to a variable - ie:
return (await extensionsApi.listInstances(projectId)).filter(...).map(...);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/extensions/runtimes/common.ts
Outdated
// Make sure the directories exist | ||
await fs.promises | ||
.mkdir(path.dirname(filePath), { recursive: true }) | ||
.then(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This .then()
should be unnecessary. Pretty sure this code works identically if you just delete it and replace the .catch with a standard try/catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Add firebase ext:sdk:install command