Skip to content
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 last_cached_at metadata column for existing shards #122

Merged
merged 12 commits into from Jan 15, 2016

Conversation

khalidharun
Copy link
Collaborator

It was discovered that existing shards that don't have a last_cached_at column (ie. created before cachemeifyoucan version 0.2.3.4) doesn't work correctly with safe_columns. That issue is now addressed in this PR.

@khalidharun
Copy link
Collaborator Author

@robertzk @kirillseva dpkg is erring. Can you take a look at why CI is broken?

@kirillseva
Copy link
Collaborator

Nice tests man!

@khalidharun
Copy link
Collaborator Author

thanks @kirillseva

@iveksl2
Copy link

iveksl2 commented Jan 14, 2016

Nice tests

describe("when safe_column is TRUE", {
test_that('it crashes when trying to expand a table on new column', {
dbtest::with_test_db({
lapply(dbListTables(test_con), function(t) dbRemoveTable(test_con, t))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to do this anymore... @peterhurford ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kirillseva Just did a little test. Tables persist between runs. So this line is still needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No you have to do this using with_test_db. But dbtest::db_test_that would do this automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterhurford oh woah.. Nice! That's really helpful. Thanks for clearing it up.

@kirillseva
Copy link
Collaborator

@khalidharun backmerge again

@khalidharun
Copy link
Collaborator Author

@kirillseva is there rebuild button for Travis? It hit a github error again.

@khalidharun
Copy link
Collaborator Author

@robertzk ready to go

robertzk added a commit that referenced this pull request Jan 15, 2016
Add last_cached_at metadata column for existing shards
@robertzk robertzk merged commit c8933c3 into robertzk:master Jan 15, 2016
@khalidharun khalidharun deleted the fix_last_cached_at branch January 15, 2016 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants