feat: resource tags in dataset

[keunsoopark]

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2091 🦕

[Linchin]

Thank you so much for adding resource tags in dataset! The PR looks great overall, I just made some minor modifications. I wonder if you could add a system test, as well as a unit test for setter with None? Please let us know if you need any help. Happy holidays!

[keunsoopark]

system test

Hi @Linchin, thanks for the review.

I added system tests in system/test_clients.py

I fixed unittests for None, and added more tests for invalid values. See test_resource_tags_setter_bad_value.

Merry Christmas and happy new year!

[Linchin]

The 3.12 system test is still failing - after trying to create the resource tag and finding that it already exists, the code tries to access it and gets PermissionDenied error, which indicates that the tag either doesn't exist, or the client doesn't have the permission to access it. I suspect there is a race condition where the tag was created by sys test 3.8 when 3.12 was trying to create it, and deleted when 3.12 tries to access it 😂 Let me rerun the tests to see if it's flaky, so we can verify the assumption.

If this is indeed the case, we may need to append the python version to the tags to avoid conflict.

[keunsoopark]

The 3.12 system test is still failing - after trying to create the resource tag and finding that it already exists, the code tries to access it and gets PermissionDenied error, which indicates that the tag either doesn't exist, or the client doesn't have the permission to access it. I suspect there is a race condition where the tag was created by sys test 3.8 when 3.12 was trying to create it, and deleted when 3.12 tries to access it 😂 Let me rerun the tests to see if it's flaky, so we can verify the assumption.

If this is indeed the case, we may need to append the python version to the tags to avoid conflict.

Thanks for a good inspiration! I agree that we should make each of test running independent. Instead of postfixing python version, I postfixed random characters to tag keys. I see it works well from my local testing.

In previous run, all tests failed all of sudden, and I could no see logs any longer. Could you check if it happens again in next CI tests?

[Linchin]

Thank you for the prompt reply! Adding a prefix lgtm. I think there was something wrong with the test infra, I hope it's just flaky and doesn't happen again.

[Linchin]

Thank you for your contribution, now all tests have passed! 🎉

LGTM.

[keunsoopark]

Thank you for your contribution, now all tests have passed! 🎉

LGTM.

Thank you so much! It was a pleasant review process with you, @Linchin :)
My motivation to this contribution is for making dbt able to attach resource tags - dbt-labs/dbt-adapters#577. As you see in this issue, it requires this functionality on tables as well.
I am looking forward to other PR - #2093 done soon, but if it takes too long time to get response, I can contribute it as well. Now I can fix this feature pretty quickly :)
Just ping me if you want me to be involved.

[Linchin]

It was a pleasure working with you! @keunsoopark

Let's first see if the original author of #2093 responses, then we can decide what to do next.