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

feat: add support for policy tags #77

Merged
merged 22 commits into from May 18, 2020
Merged

Conversation

@shollyman
Copy link
Contributor

@shollyman shollyman commented Apr 16, 2020

State: unit testing done
Pending: system tests

Fixes: #74

@shollyman
Copy link
Contributor Author

@shollyman shollyman commented Apr 17, 2020

@plamut I'm not understanding the coverage failures from the most recent run, which don't appear to be related to this PR. Any insights?

Running session cover
Creating virtual environment (virtualenv) using python3.8 in .nox/cover
pip install coverage pytest-cov
coverage report --show-missing --fail-under=100
Name                                                Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------------------------------
google/cloud/bigquery/__init__.py                      57      0      0      0   100%
google/cloud/bigquery/_helpers.py                     237      0    120      0   100%
google/cloud/bigquery/_http.py                         13      0      0      0   100%
google/cloud/bigquery/_pandas_helpers.py              262      0    125      0   100%
google/cloud/bigquery/client.py                       642      2    272      0    99%   22-23
google/cloud/bigquery/dataset.py                      252      0     61      0   100%
google/cloud/bigquery/dbapi/__init__.py                30      0      0      0   100%
google/cloud/bigquery/dbapi/_helpers.py                77      2     42      0    98%   17-18
google/cloud/bigquery/dbapi/connection.py              15      0      2      0   100%
google/cloud/bigquery/dbapi/cursor.py                 128      2     36      0    99%   21-22
google/cloud/bigquery/dbapi/exceptions.py              11      0      0      0   100%
google/cloud/bigquery/dbapi/types.py                   22      0      0      0   100%
google/cloud/bigquery/encryption_configuration.py      28      0      4      0   100%
google/cloud/bigquery/enums.py                         31      0      2      0   100%
google/cloud/bigquery/exceptions.py                     2      0      0      0   100%
google/cloud/bigquery/external_config.py              314      0     30      0   100%
google/cloud/bigquery/job.py                         1373      0    236      0   100%
google/cloud/bigquery/magics.py                       192      0     69      0   100%
google/cloud/bigquery/model.py                        158      0     24      0   100%
google/cloud/bigquery/query.py                        238      0     72      0   100%
google/cloud/bigquery/retry.py                         13      0      4      0   100%
google/cloud/bigquery/routine.py                      195      0     30      0   100%
google/cloud/bigquery/schema.py                       114      0     42      0   100%
google/cloud/bigquery/table.py                        760      0    244      0   100%
tests/unit/__init__.py                                  0      0      0      0   100%
tests/unit/enums/__init__.py                            0      0      0      0   100%
tests/unit/enums/test_standard_sql_data_types.py       33      0      8      0   100%
tests/unit/helpers.py                                   8      0      0      0   100%
tests/unit/model/__init__.py                            0      0      0      0   100%
tests/unit/model/test_model.py                        125      0      0      0   100%
tests/unit/model/test_model_reference.py               72      0      0      0   100%
tests/unit/routine/__init__.py                          0      0      0      0   100%
tests/unit/routine/test_routine.py                    114      0      0      0   100%
tests/unit/routine/test_routine_argument.py            47      0      0      0   100%
tests/unit/routine/test_routine_reference.py           72      0      0      0   100%
tests/unit/test__helpers.py                           617      0      6      0   100%
tests/unit/test__http.py                               68      0      0      0   100%
tests/unit/test__pandas_helpers.py                    435      0     40      0   100%
tests/unit/test_client.py                            3238      4    104      4    99%   7449->7450, 7450, 7499->7500, 7500, 7541->7542, 7542, 7586->7587, 7587
tests/unit/test_dataset.py                            497      0     20      0   100%
tests/unit/test_dbapi__helpers.py                     116      0     10      0   100%
tests/unit/test_dbapi_connection.py                    73      0      0      0   100%
tests/unit/test_dbapi_cursor.py                       298      0     14      0   100%
tests/unit/test_dbapi_types.py                         18      0      0      0   100%
tests/unit/test_encryption_configuration.py            75      0      0      0   100%
tests/unit/test_external_config.py                    204      0      0      0   100%
tests/unit/test_job.py                               3432      0    132      0   100%
tests/unit/test_magics.py                             748      0      4      0   100%
tests/unit/test_query.py                              625      0      4      0   100%
tests/unit/test_retry.py                               38      0      0      0   100%
tests/unit/test_schema.py                             360      0      8      0   100%
tests/unit/test_signature_compatibility.py             20      0      0      0   100%
tests/unit/test_table.py                             2089      0    100      0   100%
-----------------------------------------------------------------------------------------------
TOTAL                                               18586     10   1865      4    99%

Loading

@plamut
Copy link
Contributor

@plamut plamut commented Apr 20, 2020

@shollyman The coverage is aggregated across all unit test runs, and that final number is then checked (needs to be 100%).

There are several if-py2.7-else compatibility checks in the code, but since the Python 2.7 unit tests session encountered an error, it did not contribute to the total coverage and the coverage check failed.

It's not related to the changes here, however, as it also happens on master. Some dependencies do not build anymore on older Python versions, will investigate.

Update:
A new version of llvmlite (an indirect dependency) was released three days ago, breaking the installation on Python 2.7 and 3.5. A fix is available: #79.

Loading

Copy link
Contributor

@plamut plamut left a comment

Generally looks good. I had a few comments, but mostly nits - feel free to dismiss those if you find them too insignificant.

Adding a system test or two makes sense, indeed, awaiting that.

Loading

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
@shollyman
Copy link
Contributor Author

@shollyman shollyman commented Apr 20, 2020

There's a problem with the integration testing on the datacatalog side, tracking this internally as 154544391

Loading

@shollyman
Copy link
Contributor Author

@shollyman shollyman commented Apr 20, 2020

That said, it seems like you can avoid the integration and simply supply policy tags that match the defined resource pattern, so I'll add a quick test to do so and we can revisit once the datacatalog API issues are resolved.

Loading

Copy link
Contributor

@plamut plamut left a comment

We missed one deepcopy, sorry for not mentioning it in the initial pass, while the rest looks good. Thanks for the quick update!

Looking forward to that quick test and we should be ready to merge then.

Loading

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
Copy link
Contributor

@plamut plamut left a comment

Looks good. The several comments I made during the review have already been fixed in subsequent commits, and what's left is an edge case that seems to not be covered by a test yet.

Loading

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Show resolved Hide resolved
Loading
google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
tests/unit/test_schema.py Show resolved Hide resolved
Loading
@shollyman
Copy link
Contributor Author

@shollyman shollyman commented May 15, 2020

After wobbling a bit more, I decided that the right thing here was to make policytags immutable as it lives inside the immutable schemafield entities. Hence, ripped out the setter, and switch to/from api repr to convert between tuple and list forms.

Loading

Copy link
Contributor

@plamut plamut left a comment

Making the object immutable sounds sensible, and the changes look good. 👍

I just spotted a comment in one of the docstrings that appears to be redundant/misleading, but that's about it.

Loading

google/cloud/bigquery/schema.py Outdated Show resolved Hide resolved
Loading
plamut
plamut approved these changes May 18, 2020
@plamut plamut merged commit 38a5c01 into googleapis:master May 18, 2020
3 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants