-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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(migration): add log for values unseen in Slice.datasource_type
#23925
Conversation
session.add(slc) | ||
|
||
else: | ||
print( |
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.
Can we use a logger here? Print is a little nasty for aggregators like Datadog :P
@@ -36,6 +37,8 @@ | |||
|
|||
Base = declarative_base() | |||
|
|||
logger = logging.getLogger() |
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.
logging.getLogger(__name__)
session.add(slc) | ||
|
||
else: | ||
logger.info( |
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.
logger.warn()
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.
Nit: no f-strings in log msgs. Should be ok here as this code will only really run once :)
Codecov Report
@@ Coverage Diff @@
## master #23925 +/- ##
==========================================
- Coverage 68.10% 68.10% -0.01%
==========================================
Files 1941 1941
Lines 75020 75036 +16
Branches 8155 8155
==========================================
+ Hits 51096 51106 +10
- Misses 21841 21847 +6
Partials 2083 2083
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
||
else: | ||
logger.warn( | ||
f"unknown value detected for slc.datasource_type: {slc.datasource_type}" |
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.
I would suggest also putting a log into line 60 above in case that's where the failure is.
@hughhhh don't we need to create a new dataset from the query? Rather than just changing the datasource_type to table? |
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.
left a few comments
superset/migrations/versions/2023-03-27_12-30_7e67aecbf3f1_chart_ds_constraint.py
Show resolved
Hide resolved
superset/migrations/versions/2023-03-27_12-30_7e67aecbf3f1_chart_ds_constraint.py
Show resolved
Hide resolved
…rt_ds_constraint.py Co-authored-by: Elizabeth Thompson <[email protected]>
…rt_ds_constraint.py Co-authored-by: Elizabeth Thompson <[email protected]>
…rt_ds_constraint.py
superset/migrations/versions/2023-03-27_12-30_7e67aecbf3f1_chart_ds_constraint.py
Outdated
Show resolved
Hide resolved
…rt_ds_constraint.py
…pache#23925) Co-authored-by: Elizabeth Thompson <[email protected]> (cherry picked from commit 3dc4de4)
🏷️ preset:2023.17 |
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION