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

Command to start a local container #491

Merged
merged 19 commits into from
Dec 12, 2024
Merged

Command to start a local container #491

merged 19 commits into from
Dec 12, 2024

Conversation

cleve-fauna
Copy link
Contributor

@cleve-fauna cleve-fauna commented Dec 6, 2024

Ticket(s): FE-5990

Problem

Fauna's docker container gives users an easy way to implement a reliable inner loop development cycle. It makes it easy for them to develop schemas and programs against Fauna locally, without incurring any service costs.

Our support for the container in tooling is absent. User's have to find it and build scripts to fire it up.

Solution

Take a first step to making the container more prevalent in the tooling by making a CLI command to start the container.

Result

You can start a local Fauna with fauna local

Testing

Ran the tests. Notice how I added a bunch for the container.

Out of Scope

I need to add tests for different argv inputs, including incorrect inputs. I've put a generic skipped test to flag that for now.

@cleve-fauna cleve-fauna changed the title RFC local conatiner commands Command to start a local container Dec 10, 2024
@cleve-fauna cleve-fauna marked this pull request as ready for review December 11, 2024 22:17
@cleve-fauna cleve-fauna requested a review from a team as a code owner December 11, 2024 22:17
Copy link

@ecooper ecooper left a comment

Choose a reason for hiding this comment

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

Looking sharp.

I still want the ability to push a schema to a database (creating it if it doesn't already exist), but this is slick by itself too.

@@ -79,7 +80,7 @@
"pretest:ci": "npm run build:app",
"test:ci": "mocha --forbid-only --recursive ./test --require ./test/mocha-root-hooks.mjs --reporter mocha-multi-reporters --reporter-options configFile=./test/config/reporter.json",
"build": "npm run build:app && npm run build:sea",
"build:app": "esbuild --bundle ./src/user-entrypoint.mjs --platform=node --outfile=./dist/cli.cjs --format=cjs --inject:./sea/import-meta-url.js --define:import.meta.url=importMetaUrl --define:process.env.NODE_ENV=\\\"production\\\"",
"build:app": "esbuild --loader:.node=file --bundle ./src/user-entrypoint.mjs --platform=node --outfile=./dist/cli.cjs --format=cjs --inject:./sea/import-meta-url.js --define:import.meta.url=importMetaUrl --define:process.env.NODE_ENV=\\\"production\\\"",
Copy link

Choose a reason for hiding this comment

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

tl;dr: What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some node modules like low level crypto libraries esbuild does not bundle by default. This instructs esbuild to do so by copying the files it needs into the bundle.

containerPort,
});
logger.stderr(
`[StartContainer] Container '${containerName}' started. Monitoring HealthCheck for readiness.`,
Copy link

Choose a reason for hiding this comment

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

FWIW, we could update colorize (and codeToAnsi) to support colorizing logs in a couple lines:

https://textmate-grammars-themes.netlify.app/?theme=github-dark-high-contrast&grammar=log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea.

Comment on lines 37 to 41
image: {
describe: "The image to run locally",
type: "string",
default: "fauna/faunadb:latest",
},
Copy link

Choose a reason for hiding this comment

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

Why is this customizable?

Copy link
Contributor Author

@cleve-fauna cleve-fauna Dec 12, 2024

Choose a reason for hiding this comment

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

It probably doesn't need to be. I put it in case someone wanted to use the "nightly" container build. But taht seems like an internal concern not appropriate for this tooling.

There are specific versions published: https://hub.docker.com/r/fauna/faunadb/tags

So i could see a case for having the tag be mutable for debugging. So you'd take a --tag argument instead. But we could also just drop it and always do latest which seems more appropriate for a web service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this flag.

type: "boolean",
default: true,
},
});
Copy link

Choose a reason for hiding this comment

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

We can do this in a future PR, but I really love to see FSL directory flag and a database name flag that this command creates and pushes into.

Copy link
Contributor Author

@cleve-fauna cleve-fauna Dec 12, 2024

Choose a reason for hiding this comment

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

That's good idea for starting up dev and test environments.

Comment on lines +33 to +49
it("Creates and starts a container when none exists", async () => {
docker.pull.onCall(0).resolves();
docker.modem.followProgress.callsFake((stream, onFinished) => {
onFinished();
});
docker.listContainers.onCall(0).resolves([]);
fetch.onCall(0).resolves(f({})); // fast succeed the health check
logsStub.callsFake(async () => ({
on: () => {},
destroy: () => {},
}));
docker.createContainer.resolves({
start: startStub,
logs: logsStub,
unpause: unpauseStub,
});
await run("local", container);
Copy link
Contributor

Choose a reason for hiding this comment

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

cool tests :)

default: "8443",
},
name: {
describe: "The name to give the container",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe: "The name to give the container",
describe: "The name to give the container.",

Comment on lines +6 to +14
/**
* Ensures the container is running
* @param {string} imageName The name of the image to create the container from
* @param {string} containerName The name of the container to start
* @param {number} hostPort The port on the host machine mapped to the container's port
* @param {number} containerPort The port inside the container Fauna listens on
* @param {boolean} pull Whether to pull the latest image
* @returns {Promise<void>}
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

love the JSDoc annotations, they're really polished / helpful

Comment on lines +38 to +40
logger.stderr(
`[ContainerReady] Container '${containerName}' is up and healthy.`,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR, but this is probably a good fit for one of the verbosity level loggers (logger.debug/info/warn/error/fatal). maybe we put it on info and change the default log level to include info? going through the verbosity loggers gives users easier / better sifting (b/c you give it a component and a log level, and they can use those to filter in/out sets of logs).

Co-authored-by: echo-bravo-yahoo <[email protected]>
@cleve-fauna cleve-fauna merged commit 1801770 into v3 Dec 12, 2024
4 checks passed
@cleve-fauna cleve-fauna deleted the v3_localCommands branch December 12, 2024 01:39
@cleve-fauna cleve-fauna mentioned this pull request Dec 13, 2024
@wildemat wildemat mentioned this pull request Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants