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

Finish the database list command #448

Merged
merged 11 commits into from
Dec 3, 2024
Merged

Finish the database list command #448

merged 11 commits into from
Dec 3, 2024

Conversation

henryfauna
Copy link
Contributor

Ticket(s): FE-6126

Problem

The database list command does not support --secret or --database.

Solution

Support the --secret and --database flags by using the account API if no secret is given.

Result

The database list command supports all expected flags.

For example, the following commands should now all work:

  1. fauna database list
  2. fauna database list --secret 'my-secret'
  3. fauna database list --database 'us-std/my-db'
  4. fauna database list --json
  5. fauna database list --pageSize 10

Testing

Screenshot 2024-12-02 at 4 22 06 PM

@henryfauna henryfauna requested a review from a team as a code owner December 3, 2024 00:23
Copy link
Contributor

@cleve-fauna cleve-fauna left a comment

Choose a reason for hiding this comment

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

Questions

Comment on lines +10 to +18
const getOutputFields = (argv) => {
if (!argv.secret && !argv.database) {
// If we are listing top level databases the region group
// needs to be included as database names can be re-used across
// regions.
return ["name", "region_group"];
}
return ["name"];
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we choosing not to include has has_children, region_group and path?

I realize that the core APIs can't give us this at this time.

Is it strictly for that reason?

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 I've included region_group if listing at the top level as that is needed to disambiguate databases with the same name across region groups. Otherwise I've just kept it to name to be consistent when listing from both APIs. Theoretically you also know the region and path if listing with the --database flag.

);
}

async function listDatabasesWithAccountAPI(argv) {
const { pageSize, database, json } = argv;
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not supporting pagination?

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 didn't want to tackle that here but I can in a follow up

Comment on lines +90 to +92
"$0 database list --secret 'my-secret'",
"list all databases using the provided database secret",
Copy link
Contributor

Choose a reason for hiding this comment

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

what does fauna database list --secret 'my-secret' --database 'eu-std/example' 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.

You get this error message Cannot use both the '--secret' and '--database' options together. Please specify only one.

import { run } from "../../src/cli.mjs";
import { setupTestContainer as setupContainer } from "../../src/config/setup-test-container.mjs";

describe("database list", () => {
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 plan to test we get the output we actually we want?

Is that being saved for the integration test story?

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's what I was thinking

@henryfauna henryfauna merged commit 7fce0fa into v3 Dec 3, 2024
4 checks passed
@henryfauna henryfauna deleted the list-database branch December 3, 2024 18:59
@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
@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