-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: adding logging to validation #16527
feat: adding logging to validation #16527
Conversation
253a773
to
1088671
Compare
1088671
to
eb77e7f
Compare
@@ -908,14 +908,14 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({ | |||
/> | |||
); | |||
} | |||
|
|||
const message: Array<string> = Object.values(dbErrors); | |||
const message: Array<string> | null = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a database connection errror when test connection has failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not related to your changes, but I think we really should show all the messages, not just the first one. Left some suggestions on how to do that.
return ( | ||
<Alert | ||
type="error" | ||
css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)} | ||
message="Database Creation Error" | ||
description={message[0]} | ||
description={message?.[0] || dbErrors} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why we're showing just the first message here?
It would be better (and easy) to show all of them:
description={message?.[0] || dbErrors} | |
description={message.join('\n') || dbErrors} |
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, can you run a test and see if the error messages get logged successfully?
I think @hughhhh has some experience checking that logs are produced correctly when testing locally.
…ndex.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Codecov Report
@@ Coverage Diff @@
## master #16527 +/- ##
==========================================
- Coverage 76.63% 76.59% -0.05%
==========================================
Files 1002 1002
Lines 53680 53802 +122
Branches 6852 6861 +9
==========================================
+ Hits 41138 41208 +70
- Misses 12305 12357 +52
Partials 237 237
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@betodealmeida this is from my local logging. |
* adding logging to validation * Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> (cherry picked from commit 376c685)
* adding logging to validation * Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
* adding logging to validation * Update superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
SUMMARY
Logs the engine to the validation_parameters for better logging.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION