-
Notifications
You must be signed in to change notification settings - Fork 306
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
Conversation
@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?
|
@shollyman The coverage is aggregated across all unit test runs, and that final number is then checked (needs to be 100%). There are several It's not related to the changes here, however, as it also happens on Update: |
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.
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.
There's a problem with the integration testing on the datacatalog side, tracking this internally as 154544391 |
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. |
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 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.
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.
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.
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. |
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.
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.
State: unit testing done
Pending: system tests
Fixes: #74