-
Notifications
You must be signed in to change notification settings - Fork 1.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 missing fields to properly list partitioned tables #4306
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.
Thanks for the pr! Could you add a simple test?
Added a unit test but since everything is mocked it doesn't actually call |
There's a non-mocked integration test for list tables in ITBigQueryTest which you could add to, or write a new IT based on it |
Thanks. I wasn't able to run the ITBigQueryTests due to |
You'll need to set the environment variables |
The BigtableIT tests may require configuration via system properties. For CI, we set them here. We set |
Thanks guys, I found the env variables needed but obviously I don't have credentials to access that project so the test fails on |
The idea would be to use your own project instead to test, setting the values of the environment variables to be your own project and credentials |
@vvviiimmm, please run the following command to clean up the code formatting:
|
@vvviiimmm, please run the following command to clean up the code formatting:
|
Codecov Report
@@ Coverage Diff @@
## master #4306 +/- ##
============================================
- Coverage 49.17% 49.16% -0.01%
- Complexity 20988 21915 +927
============================================
Files 1996 2078 +82
Lines 194629 207084 +12455
Branches 21796 24084 +2288
============================================
+ Hits 95702 101813 +6111
- Misses 90817 97128 +6311
- Partials 8110 8143 +33
Continue to review full report at Codecov.
|
@JesseLovelace @kolea2 is this something we should:
|
@JesseLovelace, can you please finish this review? |
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 do need an IT request for this.
@pmakani, can you please raise a new PR with this change, and add something to the IT test to test this change?
@sduskis sure, i will add that to IT test and raise a new PR. |
This is replaced by #4701 which includes these changes + integration tests |
While getting the list of tables (list endpoint) the information about partitioning and creation time is getting lost. Somewhat related to #3097.