added passing client_tags option to Trino plugin#7633
Conversation
added the option to pass client tags to trino clusters source
RoeyoOgen
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
@kimsehwan96 saw you last touched the Trino code, would love to get a review @tsuyoshizawa would to get one too :) |
redash/query_runner/trino.py
Outdated
| elif isinstance(client_tags, list): | ||
| tags = [str(tag).strip() for tag in client_tags] |
There was a problem hiding this comment.
| elif isinstance(client_tags, list): | |
| tags = [str(tag).strip() for tag in client_tags] |
Is this elif isinstance(client_tags, list): really needed?
The configuration_schema defines client_tags as "type": "string" and ConfigurationContainer.update validates against the schema with jsonschema.validate before saving so a list values would be rejected at write time and never reach this condition!
yoshiokatsuneo
left a comment
yoshiokatsuneo
left a comment
There was a problem hiding this comment.
Thank you for your PR !
redash/query_runner/trino.py
Outdated
|
|
||
| def _get_client_tags(self): | ||
| client_tags = self.configuration.get("client_tags") | ||
| if not isinstance(client_tags, str): |
There was a problem hiding this comment.
I think just if not client_tags: will be simpler, as client_tags is always string or None and can be empty string.
tests/query_runner/test_trino.py
Outdated
| expected_catalogs = [TestTrino.catalog_name] | ||
| self.assertEqual(catalogs, expected_catalogs) | ||
|
|
||
| def test_configuration_schema_has_client_tags(self): |
There was a problem hiding this comment.
I think this test is redundant and we don't need this test as configuration_schema is just a constant.
Cheers! nice to e-meet :) finished addressing comments 🤩 |
|
@yoshiokatsuneo can you assist with the failed tests? |
|
I just confirmed that the PR works. |
3 participants
added the option to pass client tags to trino clusters source
What type of PR is this?
Description
Added the option to pass client_tags to the Trino cluster
How is this tested?
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)