-
Notifications
You must be signed in to change notification settings - Fork 14k
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: The dynamic form to connect to Snowflake DB is not returning any errors #20013
fix: The dynamic form to connect to Snowflake DB is not returning any errors #20013
Conversation
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.
LGTM and just leave a nit. Thanks for the fix.
return ( | ||
<Alert | ||
type="error" | ||
css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)} | ||
message={t('Database Creation Error')} | ||
description={message?.[0] || dbErrors} | ||
description={alertErrors?.[0]} |
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.
Looks like ?.
is an unnecessary logic and it's better to use t
function to wrap it.
Codecov Report
@@ Coverage Diff @@
## master #20013 +/- ##
==========================================
- Coverage 66.36% 66.35% -0.01%
==========================================
Files 1712 1712
Lines 64088 64089 +1
Branches 6744 6745 +1
==========================================
- Hits 42529 42528 -1
- Misses 19846 19848 +2
Partials 1713 1713
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
caf46c0
to
768cecf
Compare
return ( | ||
<Alert | ||
type="error" | ||
css={(theme: SupersetTheme) => antDErrorAlertStyles(theme)} | ||
message={t('Database Creation Error')} | ||
description={message?.[0] || dbErrors} | ||
description={t(alertErrors[0])} |
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.
Looking above at line 1095, it seems there's a condition where alertErrors
might be []
, in which case this would be translating/displaying undefined
right?
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.
we're checking for content in 1098
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.
lol 🤦
… errors (apache#20013) * fix: The dynamic form to connect to Snowflake DB is not returning any errors * PR comment (cherry picked from commit c6dd7fe)
🏷️ preset:2022.19 |
… errors (apache#20013) * fix: The dynamic form to connect to Snowflake DB is not returning any errors * PR comment
SUMMARY
There are two ways to create a connection with a Snowflake DB:
Using the dynamic form.
Using a SQLAlchemy URI string.
However, the dynamic form is not displaying authentication errors to the user, in case the authentication fails.
This PR reintroduces the fixes from #19491 that were overridden in #19165
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
161301153-772e3b64-5ff2-4ce0-a41b-372821481fe5.mov
After:
new.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION