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

fix Connected to undefined message #284

Merged
merged 4 commits into from
Oct 10, 2023
Merged

fix Connected to undefined message #284

merged 4 commits into from
Oct 10, 2023

Conversation

fauna-chase
Copy link
Contributor

@fauna-chase fauna-chase commented Oct 10, 2023

ENG-5630
This will display the endpoint and database being connected to when ever a CLI command is executed.

p4 » local-shell schema diff
Connected to endpoint: cli_test-us database: cli_bugbash
No schema differences
p4 » local-shell shell
Connected to endpoint: cli_test-us database: cli_bugbash
Type Ctrl+D or .exit to exit the shell
>

@fauna-chase fauna-chase requested a review from macmv October 10, 2023 01:20
@fauna-chase
Copy link
Contributor Author

copying my jira comment here:

the previous impl was just printing the url it was connecting to. I updated the current impl to do that, but do we think printing that url has any value? for customers it will always be https://db.fauna.com or localhost feels like we could just strip it out entirely.

One thing that does seem valuable is printing the database that is being connected to. Should we update it to do that?

@macmv
Copy link
Contributor

macmv commented Oct 10, 2023

I like the idea of printing the current endpoint and database that its connected to. URL seems nice to have too, if we can fit all 3 of those in that one-liner.

`Could not Connect to ${stringifyEndpoint(
connectionOptions
)} Unauthorized Secret`
`Could not Connect to ${connectionOptions.url} Unauthorized Secret`
Copy link
Contributor

@macmv macmv Oct 10, 2023

Choose a reason for hiding this comment

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

Suggested change
`Could not Connect to ${connectionOptions.url} Unauthorized Secret`
`Could not connect to ${connectionOptions.url}: unauthorized secret`

also it might be nice to print the secret there? not sure if we want to expose that though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah I'm not sure that we want to do that, my bias would be to just leave it out

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me

ENG-5630
This will display the endpoint and database being connected to when ever a CLI command is executed.
@fauna-chase
Copy link
Contributor Author

I like the idea of printing the current endpoint and database that its connected to. URL seems nice to have too, if we can fit all 3 of those in that one-liner.

i updated this pr to print the current endpoint and database for every command. Also seems useful when doing schema commands and such

Copy link
Contributor

@macmv macmv left a comment

Choose a reason for hiding this comment

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

looks good to me, except for name being optional. IMO it should always be set.

src/lib/config/root-config.ts Show resolved Hide resolved
return new Endpoint({
name: name,
Copy link
Contributor

Choose a reason for hiding this comment

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

style:

Suggested change
name: name,
name,

src/lib/config/root-config.ts Show resolved Hide resolved
test/commands/endpoint.test.ts Show resolved Hide resolved
For eval, we currently output json.  Entering a log line can make the output invalid json and could possibly break tooling people have around it.  This allows commands to opt out of the connection info logging.
@fauna-chase
Copy link
Contributor Author

looks good to me, except for name being optional. IMO it should always be set.

if you provide a secret as a flag we create an endpoint without a name.

@macmv
Copy link
Contributor

macmv commented Oct 10, 2023

oh yup nevermind

@fauna-chase fauna-chase merged commit c87a0c9 into main Oct 10, 2023
@fauna-chase fauna-chase deleted the connect-undefined branch October 10, 2023 21:19
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.

2 participants