-
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
Add synctable command into caravel #977
Conversation
add synctable prefix option
Please don't open new pull requests, just push fixes on the same branch, they'll eventually be squashed at merge time :) |
Also please work on a branch and not on master, it'll be easier :) |
@@ -125,6 +125,35 @@ def refresh_druid(): | |||
"[" + cluster.cluster_name + "]") | |||
session.commit() | |||
|
|||
@manager.option( | |||
'-p', '--prefix', default='data_', |
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.
just default to None and if it's None just don't filter
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 think no filter is not good, because main database (for caravel system use) have many table not store data, It doesn't need sync.
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.
Well, first thing is that you should not install caravel on the same database of your data. Second the feature should be generic and not tied to your own usage :)
Hi @xrmx I am sorry about use master branch!! Now you recommand remove this pr and use another branch send or? Sorry I am a beginner at open source community, I am not sure how to do are good. |
Nope, this time stay on the master branch. After this is merged though you'll have to reset the master branch to the upstream one. It's awesome you opened a pull request, please continue to do that 🎉 |
I add option to specifies database and make prefix default is empty string If prefix and db not set, will show warning message to checking. |
''' Sync DB Table with Caravel Table''' | ||
print("") | ||
|
||
if (prefix == "" and database is None) or (database == 'main' and prefix == ""): |
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.
why this main special case?
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.
because if you use cmd to load examples, the db name: main
and the main db is same as Caravel core db
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.
Thanks for the explaination. Shouldn't we just reject 'main' as database name and make database mandatory?
check_status = raw_input("Are you sure do this? (Y/N)").lower().strip() | ||
if check_status not in ['y', 'yes']: | ||
print("Exit sync table") | ||
exit() |
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 usual to use exit
in python
Sorry this never made it in, I'm cleaning up older PRs with merge conflicts at the moment. Please resubmit the PR if you want this feature to make it in. |
…pache#977) * feat: change font size on responsive * remove logs * Sunburst * WIP * Sankey label hide * Format correct object * add styled components * Replace document with container * Return overlapping elements * Drag * Fix lint * Fix test & make tooltip absoliute based on cursor position * Update storybook for charts * Resizable in separate page * Fix responsive class for sunburst * type
…pache#977) * feat: change font size on responsive * remove logs * Sunburst * WIP * Sankey label hide * Format correct object * add styled components * Replace document with container * Return overlapping elements * Drag * Fix lint * Fix test & make tooltip absoliute based on cursor position * Update storybook for charts * Resizable in separate page * Fix responsive class for sunburst * type
…pache#977) * feat: change font size on responsive * remove logs * Sunburst * WIP * Sankey label hide * Format correct object * add styled components * Replace document with container * Return overlapping elements * Drag * Fix lint * Fix test & make tooltip absoliute based on cursor position * Update storybook for charts * Resizable in separate page * Fix responsive class for sunburst * type
…pache#977) * feat: change font size on responsive * remove logs * Sunburst * WIP * Sankey label hide * Format correct object * add styled components * Replace document with container * Return overlapping elements * Drag * Fix lint * Fix test & make tooltip absoliute based on cursor position * Update storybook for charts * Resizable in separate page * Fix responsive class for sunburst * type
Sometime we have many table need insert to caravel tables, I write a command can help people to do this, set a table name prefix or default is data_ , this cmd will scan caravel has table and database all table, compare and insert it.