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

Create Database Command #435

Merged
merged 6 commits into from
Nov 25, 2024
Merged

Create Database Command #435

merged 6 commits into from
Nov 25, 2024

Conversation

henryfauna
Copy link
Contributor

@henryfauna henryfauna commented Nov 23, 2024

Ticket(s): FE-6125

Problem

The create database command is stubbed out and does not actually do anything.

Solution

  • Wire it up with the Fauna v10 driver and run Database.create.
  • Accept typechecked, protected and priority options.
  • Set up some common functions to interact with the v10 driver.

Result

The database create command works using a secret. Providing a database name instead of a secret is not yet supported.

Testing

Added tests to ensure the command executes the correct query.

src/lib/fauna.mjs Outdated Show resolved Hide resolved
if (!_client && url && secret) {
_client = await getV10Client({ url, secret });
} else if (!_client) {
throw new Error("No client provided and no url and secret provided");
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about pushing the validation all the way down to getV10Client? there's validation at the very top of the stack (argv); might make more sense for this validation to live at the very bottom of the stack and let the middle parts assume valid input.

Copy link
Contributor Author

@henryfauna henryfauna Nov 23, 2024

Choose a reason for hiding this comment

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

Sure I can push it to getV10Client. What's the downside of doing validation in runV10Query?

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 kept a bit of validation in runV10Query just to sanity check the args - let me know if you think there's a better way to do this though

@@ -0,0 +1,31 @@
export const getV10Client = async ({ url, secret }) => {
const { Client } = (await import("fauna")).default;
Copy link
Collaborator

Choose a reason for hiding this comment

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

also, i totally lied to you about there not being an existing v10 client usage in the project :(

the eval command has its own v10 query path that's used by it and the shell command:
https://github.com/fauna/fauna-shell/blob/v3/src/commands/eval.mjs#L101

i think it would make the most sense for eval and shell to only have the formatting logic, and for us to have only one implementation of the client/network stack. take a look at getSimpleClient and performQuery and see what you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that v10 path is not using the driver though so that's why I set up these new helpers. I definitely agree about having a single implementation, I tried consolidating things a bit but it was proving to be difficult. Something to sort out next week :)

@henryfauna henryfauna changed the title setup create database Create Database Command Nov 23, 2024
@henryfauna henryfauna marked this pull request as ready for review November 23, 2024 19:12
@henryfauna henryfauna requested a review from a team as a code owner November 23, 2024 19:12
src/commands/database/create.mjs Outdated Show resolved Hide resolved
test/database/create.mjs Outdated Show resolved Hide resolved
test/database/create.mjs Outdated Show resolved Hide resolved
@henryfauna henryfauna merged commit 10dabb3 into v3 Nov 25, 2024
4 checks passed
@henryfauna henryfauna deleted the create-database branch November 25, 2024 20:42
@cleve-fauna cleve-fauna mentioned this pull request Dec 5, 2024
This was referenced Dec 6, 2024
@cleve-fauna cleve-fauna mentioned this pull request Dec 13, 2024
@mwilde345 mwilde345 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