Skip to content

Web: Provide accurate actionable steps with duplicate db name error#26116

Merged
kimlisa merged 10 commits intomasterfrom
lisa/provide-better-error-msg
May 16, 2023
Merged

Web: Provide accurate actionable steps with duplicate db name error#26116
kimlisa merged 10 commits intomasterfrom
lisa/provide-better-error-msg

Conversation

@kimlisa
Copy link
Copy Markdown
Contributor

@kimlisa kimlisa commented May 12, 2023

Part of discover work to improve rds flow:

  • Before, we gave a user ambiguous "duplicate db name, use a different name and try again". Now we do some tests to provide a user more actionable steps to take depending on if a db server exists for the database or not
  • We now pre-check if the rds db's we fetched from aws has already been enrolled and if enrolled, prevent user from re-enrolling by making table row disabled

Screenshot

note: the tctl command is actually tctl rm db/<dbname> i just took the screenshot before i caught it

Self-hosted, duplicate db with a database server:
image

Self-hosted, duplicated db (no db server)
image

aws rds, dup db (no db server)
image

aws rds, dup db with a db server
image

disable row:
image

@github-actions github-actions Bot requested review from gzdunek and ryanclark May 12, 2023 08:28
@kimlisa kimlisa force-pushed the lisa/provide-better-error-msg branch 2 times, most recently from 608aee5 to e5f104a Compare May 12, 2023 09:04
Copy link
Copy Markdown
Member

@ryanclark ryanclark left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Boolean() here is redundant as you're using &&

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i get this type error if i don't cast it:

Argument of type 'string' is not assignable to parameter of type 'boolean'

Comment thread web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts Outdated
@gzdunek
Copy link
Copy Markdown
Contributor

gzdunek commented May 12, 2023

I will take a look on Monday.

Comment thread web/packages/shared/utils/errorType.ts Outdated
Comment thread web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts Outdated
Comment thread web/packages/teleport/src/Discover/Database/EnrollRdsDatabase/RdsDatabaseList.tsx Outdated
@gzdunek
Copy link
Copy Markdown
Contributor

gzdunek commented May 15, 2023

Nit: I think that long error messages look better as sentences:

Failed to register database: a database with the name "${dbName}" is a part of this cluster, set 
"teleport.dev/database_name" tag on this RDS instance to use a different name and try again.

(but if other error messages are not written this way then let it stay as it is)

@kimlisa kimlisa force-pushed the lisa/provide-better-error-msg branch from e5f104a to 5391fc1 Compare May 16, 2023 02:15
@kimlisa
Copy link
Copy Markdown
Contributor Author

kimlisa commented May 16, 2023

Nit: I think that long error messages look better as sentences:

Failed to register database: a database with the name "${dbName}" is a part of this cluster, set 
"teleport.dev/database_name" tag on this RDS instance to use a different name and try again.

(but if other error messages are not written this way then let it stay as it is)

i left it as is, i think we want errors to be all lower cased

@kimlisa kimlisa force-pushed the lisa/provide-better-error-msg branch from 82974cb to f42374f Compare May 16, 2023 18:45
@kimlisa kimlisa added this pull request to the merge queue May 16, 2023
@kimlisa kimlisa removed this pull request from the merge queue due to a manual request May 16, 2023
@kimlisa kimlisa enabled auto-merge May 16, 2023 19:04
@kimlisa kimlisa added this pull request to the merge queue May 16, 2023
Merged via the queue into master with commit 9bac74a May 16, 2023
@kimlisa kimlisa deleted the lisa/provide-better-error-msg branch May 16, 2023 19:23
@public-teleport-github-review-bot
Copy link
Copy Markdown

@kimlisa See the table below for backport results.

Branch Result
branch/v12 Create PR
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants